diff --git a/.rubocop.yml b/.rubocop.yml index fd05c0f..5118607 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -81,6 +81,8 @@ Style/SymbolArray: RSpec/ExampleLength: Max: 11 + Exclude: + - spec/rubocop/cop/thread_safety/rack_middleware_instance_variable_spec.rb RSpec/ContextWording: Exclude: diff --git a/CHANGELOG.md b/CHANGELOG.md index 0651690..f1a6f4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,8 @@ ## master (unreleased) -* [#52](https://github.com/rubocop/rubocop-thread_safety/pull/52): Drop support for RuboCop older than 1.48. ([@viralpraxis](https://github.com/viralpraxis)) +* [#54](https://github.com/rubocop/rubocop-thread_safety/pull/54): Drop support for RuboCop older than 1.48. ([@viralpraxis](https://github.com/viralpraxis)) +* [#52](https://github.com/rubocop/rubocop-thread_safety/pull/52): Add new `RackMiddlewareInstanceVariable` cop to detect instance variables in Rack middleware. ([@viralpraxis](https://github.com/viralpraxis)) * [#48](https://github.com/rubocop/rubocop-thread_safety/pull/48): Do not report instance variables in `ActionDispatch` callbacks in singleton methods. ([@viralpraxis](https://github.com/viralpraxis)) * [#43](https://github.com/rubocop/rubocop-thread_safety/pull/43): Make detection of ActiveSupport's `class_attribute` configurable. ([@viralpraxis](https://github.com/viralpraxis)) * [#42](https://github.com/rubocop/rubocop-thread_safety/pull/42): Fix some `InstanceVariableInClassMethod` cop false positive offenses. ([@viralpraxis](https://github.com/viralpraxis)) diff --git a/config/default.yml b/config/default.yml index 4f844e5..bb4af3c 100644 --- a/config/default.yml +++ b/config/default.yml @@ -35,3 +35,13 @@ ThreadSafety/NewThread: ThreadSafety/DirChdir: Description: Avoid using `Dir.chdir` due to its process-wide effect. Enabled: true + +ThreadSafety/RackMiddlewareInstanceVariable: + Description: Avoid instance variables in Rack middleware. + Enabled: true + Include: + - 'app/middleware/**/*.rb' + - 'lib/middleware/**/*.rb' + - 'app/middlewares/**/*.rb' + - 'lib/middlewares/**/*.rb' + AllowedIdentifiers: [] diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 3bc2787..f868ec7 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -5,3 +5,4 @@ * xref:cops_threadsafety.adoc#threadsafetymutableclassinstancevariable[ThreadSafety/MutableClassInstanceVariable] * xref:cops_threadsafety.adoc#threadsafetynewthread[ThreadSafety/NewThread] * xref:cops_threadsafety.adoc#threadsafetydirchdir[ThreadSafety/DirChdir] +* xref:cops_threadsafety.adoc#threadsafetyrackmiddlewareinstancevariable[ThreadSafety/RackMiddlewareInstanceVariable] diff --git a/docs/modules/ROOT/pages/cops_threadsafety.adoc b/docs/modules/ROOT/pages/cops_threadsafety.adoc index 676057c..3ed2cee 100644 --- a/docs/modules/ROOT/pages/cops_threadsafety.adoc +++ b/docs/modules/ROOT/pages/cops_threadsafety.adoc @@ -267,3 +267,78 @@ Let a framework like Sidekiq handle the threads. # bad Thread.new { do_work } ---- + +== ThreadSafety/RackMiddlewareInstanceVariable + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Enabled +| Yes +| No +| - +| - +|=== + +Avoid instance variables in rack middleware. + +Middlewares are initialized once, meaning any instance variables are shared between executor threads. +To avoid potential race conditions, it's recommended to design middlewares to be stateless +or to implement proper synchronization mechanisms. + +=== Examples + +[source,ruby] +---- +# bad +class CounterMiddleware + def initialize(app) + @app = app + @counter = 0 + end + + def call(env) + app.call(env) + ensure + @counter += 1 + end +end + +# good +class CounterMiddleware + def initialize(app) + @app = app + @counter = Concurrent::AtomicReference.new(0) + end + + def call(env) + app.call(env) + ensure + @counter.update { |ref| ref + 1 } + end +end + +class IdentityMiddleware + def initialize(app) + @app = app + end + + def call(env) + app.call(env) + end +end +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| Include +| `+app/middleware/**/*.rb+`, `+lib/middleware/**/*.rb+`, `+app/middlewares/**/*.rb+`, `+lib/middlewares/**/*.rb+` +| Array + +| AllowedIdentifiers +| `[]` +| Array +|=== diff --git a/lib/rubocop-thread_safety.rb b/lib/rubocop-thread_safety.rb index 78866a6..ccd0e72 100644 --- a/lib/rubocop-thread_safety.rb +++ b/lib/rubocop-thread_safety.rb @@ -8,8 +8,11 @@ RuboCop::ThreadSafety::Inject.defaults! +require 'rubocop/cop/mixin/operation_with_threadsafe_result' + require 'rubocop/cop/thread_safety/instance_variable_in_class_method' require 'rubocop/cop/thread_safety/class_and_module_attributes' require 'rubocop/cop/thread_safety/mutable_class_instance_variable' require 'rubocop/cop/thread_safety/new_thread' require 'rubocop/cop/thread_safety/dir_chdir' +require 'rubocop/cop/thread_safety/rack_middleware_instance_variable' diff --git a/lib/rubocop/cop/mixin/operation_with_threadsafe_result.rb b/lib/rubocop/cop/mixin/operation_with_threadsafe_result.rb new file mode 100644 index 0000000..fd8ce18 --- /dev/null +++ b/lib/rubocop/cop/mixin/operation_with_threadsafe_result.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Common functionality for checking if a well-known operation + # produces an object with thread-safe semantics. + module OperationWithThreadsafeResult + extend NodePattern::Macros + + # @!method operation_produces_threadsafe_object?(node) + def_node_matcher :operation_produces_threadsafe_object?, <<~PATTERN + { + (send (const {nil? cbase} :Queue) :new ...) + (send + (const (const {nil? cbase} :ThreadSafe) {:Hash :Array}) + :new ...) + (block + (send + (const (const {nil? cbase} :ThreadSafe) {:Hash :Array}) + :new ...) + ...) + (send (const (const {nil? cbase} :Concurrent) _) :new ...) + (block + (send (const (const {nil? cbase} :Concurrent) _) :new ...) + ...) + (send (const (const (const {nil? cbase} :Concurrent) _) _) :new ...) + (block + (send + (const (const (const {nil? cbase} :Concurrent) _) _) + :new ...) + ...) + (send + (const (const (const (const {nil? cbase} :Concurrent) _) _) _) + :new ...) + (block + (send + (const (const (const (const {nil? cbase} :Concurrent) _) _) _) + :new ...) + ...) + } + PATTERN + end + end +end diff --git a/lib/rubocop/cop/thread_safety/mutable_class_instance_variable.rb b/lib/rubocop/cop/thread_safety/mutable_class_instance_variable.rb index d82f1d5..808b1a3 100644 --- a/lib/rubocop/cop/thread_safety/mutable_class_instance_variable.rb +++ b/lib/rubocop/cop/thread_safety/mutable_class_instance_variable.rb @@ -74,8 +74,10 @@ module ThreadSafety # end class MutableClassInstanceVariable < Base extend AutoCorrector + include FrozenStringLiteral include ConfigurableEnforcedStyle + include OperationWithThreadsafeResult MSG = 'Freeze mutable objects assigned to class instance variables.' FROZEN_STRING_LITERAL_TYPES_RUBY27 = %i[str dstr].freeze @@ -241,39 +243,6 @@ def correct_splat_expansion(corrector, expr, splat_value) } PATTERN - # @!method operation_produces_threadsafe_object?(node) - def_node_matcher :operation_produces_threadsafe_object?, <<~PATTERN - { - (send (const {nil? cbase} :Queue) :new ...) - (send - (const (const {nil? cbase} :ThreadSafe) {:Hash :Array}) - :new ...) - (block - (send - (const (const {nil? cbase} :ThreadSafe) {:Hash :Array}) - :new ...) - ...) - (send (const (const {nil? cbase} :Concurrent) _) :new ...) - (block - (send (const (const {nil? cbase} :Concurrent) _) :new ...) - ...) - (send (const (const (const {nil? cbase} :Concurrent) _) _) :new ...) - (block - (send - (const (const (const {nil? cbase} :Concurrent) _) _) - :new ...) - ...) - (send - (const (const (const (const {nil? cbase} :Concurrent) _) _) _) - :new ...) - (block - (send - (const (const (const (const {nil? cbase} :Concurrent) _) _) _) - :new ...) - ...) - } - PATTERN - # @!method range_enclosed_in_parentheses?(node) def_node_matcher :range_enclosed_in_parentheses?, <<~PATTERN (begin ({irange erange} _ _)) diff --git a/lib/rubocop/cop/thread_safety/rack_middleware_instance_variable.rb b/lib/rubocop/cop/thread_safety/rack_middleware_instance_variable.rb new file mode 100644 index 0000000..c864b43 --- /dev/null +++ b/lib/rubocop/cop/thread_safety/rack_middleware_instance_variable.rb @@ -0,0 +1,120 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module ThreadSafety + # Avoid instance variables in rack middleware. + # + # Middlewares are initialized once, meaning any instance variables are shared between executor threads. + # To avoid potential race conditions, it's recommended to design middlewares to be stateless + # or to implement proper synchronization mechanisms. + # + # @example + # # bad + # class CounterMiddleware + # def initialize(app) + # @app = app + # @counter = 0 + # end + # + # def call(env) + # app.call(env) + # ensure + # @counter += 1 + # end + # end + # + # # good + # class CounterMiddleware + # def initialize(app) + # @app = app + # @counter = Concurrent::AtomicReference.new(0) + # end + # + # def call(env) + # app.call(env) + # ensure + # @counter.update { |ref| ref + 1 } + # end + # end + # + # class IdentityMiddleware + # def initialize(app) + # @app = app + # end + # + # def call(env) + # app.call(env) + # end + # end + class RackMiddlewareInstanceVariable < Base + include AllowedIdentifiers + include OperationWithThreadsafeResult + + MSG = 'Avoid instance variables in Rack middleware.' + + RESTRICT_ON_SEND = %i[instance_variable_get instance_variable_set].freeze + + # @!method rack_middleware_like_class?(node) + def_node_matcher :rack_middleware_like_class?, <<~MATCHER + (class (const nil? _) nil? (begin <(def :initialize (args (arg _)+) ...) (def :call (args (arg _)) ...) ...>)) + MATCHER + + # @!method app_variable(node) + def_node_search :app_variable, <<~MATCHER + (def :initialize (args (arg $_) ...) `(ivasgn $_ (lvar $_))) + MATCHER + + def on_class(node) + return unless rack_middleware_like_class?(node) + + constructor_method = find_constructor_method(node) + return unless (application_variable = extract_application_variable_from_contructor_method(constructor_method)) + + safe_variables = extract_safe_variables_from_constructor_method(constructor_method) + + node.each_node(:def) do |def_node| + def_node.each_node(:ivasgn, :ivar) do |ivar_node| + variable, = ivar_node.to_a + if variable == application_variable || safe_variables.include?(variable) || allowed_identifier?(variable) + next + end + + add_offense ivar_node + end + end + end + + def on_send(node) + argument = node.first_argument + + return unless argument&.sym_type? || argument&.str_type? + return if allowed_identifier?(argument.value) + + add_offense node + end + + private + + def find_constructor_method(class_node) + class_node + .each_node(:def) + .find { |node| node.method?(:initialize) && node.arguments.size >= 1 } + end + + def extract_application_variable_from_contructor_method(constructor_method) + constructor_method + .then { |node| app_variable(node) } + .then { |variables| variables.first[1] if variables.first } + end + + def extract_safe_variables_from_constructor_method(constructor_method) + constructor_method + .each_node(:ivasgn) + .select { |ivasgn_node| operation_produces_threadsafe_object?(ivasgn_node.to_a[1]) } + .map { _1.to_a[0] } + end + end + end + end +end diff --git a/spec/rubocop/cop/thread_safety/rack_middleware_instance_variable_spec.rb b/spec/rubocop/cop/thread_safety/rack_middleware_instance_variable_spec.rb new file mode 100644 index 0000000..8bd6296 --- /dev/null +++ b/spec/rubocop/cop/thread_safety/rack_middleware_instance_variable_spec.rb @@ -0,0 +1,410 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::ThreadSafety::RackMiddlewareInstanceVariable, :config do + let(:msg) { 'Avoid instance variables in Rack middleware.' } + + context 'with unrelated source' do + it { expect_no_offenses '' } + + specify do + expect_no_offenses(<<~RUBY) + class SomeClass + def initialize(user) + @user = user + end + end + RUBY + end + + specify do + expect_no_offenses(<<~RUBY) + class SomeClass + def initialize(user, context) + @user = user + @context = context + end + end + RUBY + end + + specify do + expect_no_offenses(<<~RUBY) + class SomeClass + def call(env) + @env = env + end + end + RUBY + end + + specify do + expect_no_offenses(<<~RUBY) + class SomeClass + def initialize(user, context) + @user = user + @context = context + end + + def call + [@user, @context] + end + end + RUBY + end + + specify do + expect_no_offenses(<<~RUBY) + class SomeClass + def initialize(app) + @user = User.new + end + + def call(env) + @x = TOPLEVEL_BINDING + end + end + RUBY + end + + specify do + expect_no_offenses(<<~RUBY) + class SomeClass + def initialize(app) + @app = app + @user = User.new + end + + def call(env, user) + @x = TOPLEVEL_BINDING + end + end + RUBY + end + end + + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + end + + def call(env) + @app.call(env) + end + end + RUBY + end + + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + @foo = 1 + ^^^^^^^^ #{msg} + end + + def call(env) + @app.call(env) + p @foo + ^^^^ #{msg} + end + end + RUBY + + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + @counter = 0 + ^^^^^^^^^^^^ #{msg} + end + + def call(env) + @app.call(env) + ensure + @counter += 1 + ^^^^^^^^ #{msg} + end + end + RUBY + end + + it 'registers an offense with thread-safe wrappers' do + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + @counter = Concurrent::AtomicReference.new(0) + @unsafe_counter = 0 + ^^^^^^^^^^^^^^^^^^^ Avoid instance variables in Rack middleware. + end + + def call(env) + @app.call(env) + ensure + @unsafe_counter += 1 + ^^^^^^^^^^^^^^^ Avoid instance variables in Rack middleware. + @counter.update { |ref| ref + 1 } + end + end + RUBY + end + + it 'registers an offense with mismatched local and instance variables' do + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app) + @foo = fsa + ^^^^^^^^^^ #{msg} + @a = app + end + + def call(env) + @a.call(env) + end + end + RUBY + end + + it 'registers an offense for nested middleware' do + expect_offense(<<~RUBY) + module MyMiddlewares + class TestMiddleware + def initialize(app) + @app = app + @foo = 1 + ^^^^^^^^ #{msg} + end + + def call(env) + @app.call(env) + end + end + end + RUBY + end + + it 'registers an offense for multiple middlewares' do + expect_offense(<<~RUBY) + module MyMiddlewares + class TestMiddleware + def initialize(app) + @app = app + @foo = 1 + ^^^^^^^^ #{msg} + end + + def call(env) + @app.call(env) + end + end + + class TestMiddleware2 + def initialize(app) + @app = app + @foo = 1 + ^^^^^^^^ #{msg} + end + + def call(env) + @app.call(env) + end + end + end + RUBY + end + + it 'registers an offense for extra methods' do + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + end + + def call(env) + @app.call(env) + @a = 1 + ^^^^^^ #{msg} + end + + def foo + @a = 1 + ^^^^^^ #{msg} + end + end + RUBY + + expect_offense(<<~RUBY) + class TestMiddleware + def foo + @a = 1 + ^^^^^^ #{msg} + end + + def initialize(app) + @app = app + end + + def call(env) + @app.call(env) + end + end + RUBY + + expect_offense(<<~RUBY) + class TestMiddleware + def foo + @a = 1 + ^^^^^^ #{msg} + end + + def initialize(app) + @app = app + end + + def call(env) + @app.call(env) + end + + def bar + @b = 1 + ^^^^^^ #{msg} + end + end + RUBY + end + + it 'registers an offense with `call` before constructor definition' do + expect_offense(<<~RUBY) + class TestMiddleware + def call(env) + @app.call(env) + end + + def initialize(app) + @app = app + @foo = 1 + ^^^^^^^^ #{msg} + end + end + RUBY + end + + context 'with `instance_variable_set` and `instance_variable_get` methods' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + instance_variable_set(:counter, 1) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} + end + + def call(env) + @app.call(env) + instance_variable_get("@counter") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} + end + end + RUBY + end + + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + instance_variable_set + end + + def call(env) + @app.call(env) + instance_variable_get + end + end + RUBY + + expect_no_offenses(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + instance_variable_set(1) + end + + def call(env) + @app.call(env) + instance_variable_get({}) + end + end + RUBY + end + + context 'with non-empty `AllowedIdentifiers` config' do + let(:cop_config) do + { 'AllowedIdentifiers' => ['options'] } + end + + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + class TestMiddleware + def initialize(app) + @app = app + instance_variable_set(:@options, {}) + end + + def call(env) + @app.call(env) + end + end + RUBY + end + end + end + + context 'with non-empty `AllowedIdentifiers` config' do + let(:cop_config) do + { 'AllowedIdentifiers' => ['options'] } + end + + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestMiddleware + def call(env) + @app.call(env) + end + + def initialize(app) + @app = app + @some_var = 2 + ^^^^^^^^^^^^^ #{msg} + @options = 1 + end + end + RUBY + end + end + + context 'with middleware with provided options' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class TestMiddleware + def initialize(app, options) + @app = app + @options = options + ^^^^^^^^^^^^^^^^^^ #{msg} + end + + def call(env) + if options[:noop] + [200, {}, []] + else + @app.call(env) + end + end + end + RUBY + end + end +end