diff --git a/rubocop/cop/gitlab/rescue_standard_error_without_tracking.rb b/rubocop/cop/gitlab/rescue_standard_error_without_tracking.rb new file mode 100644 index 0000000000000000000000000000000000000000..8d0923b2798a3f88377d70282dea38685ca52849 --- /dev/null +++ b/rubocop/cop/gitlab/rescue_standard_error_without_tracking.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + # Checks for rescuing StandardError without logging or tracking the error. + # + # Rescuing StandardError without logging it creates "silent failures" that make + # debugging nearly impossible since you lose all visibility into what went wrong + # and when. Instead we should try to rescue specific errors whenever possible. + # + # If there is a reason to rescue StandardError we need to use + # Gitlab::ErrorTracking.track_and_raise_for_dev immediately unless the error + # is re-raised. This reports the error to Sentry tracking and logs it. + # + # @example + # + # # bad + # begin + # do_something + # rescue StandardError => e + # return nil + # end + # + # # bad + # begin + # do_something + # rescue => e + # return nil + # end + # + # # good + # begin + # do_something + # rescue StandardError => e + # Gitlab::ErrorTracking.track_and_raise_for_dev(e) + # return nil + # end + # + # # good + # begin + # do_something + # rescue StandardError => e + # logger.error("Something went wrong: #{e.message}") + # return nil + # end + # + # # good + # begin + # do_something + # rescue StandardError => e + # raise + # end + # + # # good - specific error types are allowed + # begin + # do_something + # rescue ActiveRecord::RecordNotFound => e + # return nil + # end + # + class RescueStandardErrorWithoutTracking < RuboCop::Cop::Base + MSG = 'Avoid rescuing StandardError without logging or tracking. ' \ + 'Use Gitlab::ErrorTracking.track_and_raise_for_dev or add logging, ' \ + 'or rescue specific error types instead.' + + # Matches rescue blocks that catch StandardError (explicitly or implicitly) + # @!method rescues_standard_error?(node) + def_node_matcher :rescues_standard_error?, <<~PATTERN + (resbody + ${ + nil? + (array (const {nil? cbase} :StandardError)) + (array ... (const {nil? cbase} :StandardError) ...) + } + $_ + $... + ) + PATTERN + + # Matches calls to Gitlab::ErrorTracking.track_and_raise_for_dev + # @!method gitlab_error_tracking_call?(node) + def_node_matcher :gitlab_error_tracking_call?, <<~PATTERN + (send + (const + (const {nil? cbase} :Gitlab) + :ErrorTracking + ) + :track_and_raise_for_dev + ... + ) + PATTERN + + # Matches logging calls (logger.error, Rails.logger.error, etc.) + # @!method logging_call?(node) + def_node_matcher :logging_call?, <<~PATTERN + { + (send (send _ :logger) {:error :warn :info :debug :fatal} ...) + (send (send (const {nil? cbase} :Rails) :logger) {:error :warn :info :debug :fatal} ...) + (send (const {nil? cbase} :Rails) :logger ...) + (send _ {:error :warn :info :debug :fatal} ...) + } + PATTERN + + # Matches raise statements + # @!method raise_call?(node) + def_node_matcher :raise_call?, <<~PATTERN + (send nil? :raise ...) + PATTERN + + def on_resbody(node) + rescues_standard_error?(node) do |exception_types, _variable, body| + # Skip if rescuing specific error types along with StandardError + next if exception_types&.array_type? && + exception_types.children.any? { |child| !standard_error_const?(child) } + + # Check if the rescue block has proper error handling + next if has_proper_error_handling?(body) + + add_offense(node.loc.keyword) + end + end + + private + + def standard_error_const?(node) + return false unless node.const_type? + + node.const_name == 'StandardError' + end + + def has_proper_error_handling?(body_nodes) + return false if body_nodes.empty? + + body_nodes.any? do |node| + has_error_tracking_or_logging?(node) + end + end + + def has_error_tracking_or_logging?(node) + case node.type + when :send + gitlab_error_tracking_call?(node) || logging_call?(node) || raise_call?(node) + when :block, :begin + node.children.any? { |child| has_error_tracking_or_logging?(child) } + when :if, :case + # Check all branches + node.children.compact.any? { |child| has_error_tracking_or_logging?(child) } + else + false + end + end + end + end + end +end diff --git a/spec/rubocop/cop/gitlab/rescue_standard_error_without_tracking_spec.rb b/spec/rubocop/cop/gitlab/rescue_standard_error_without_tracking_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b63487aeb848713f4283514dbcdce7bce1126d22 --- /dev/null +++ b/spec/rubocop/cop/gitlab/rescue_standard_error_without_tracking_spec.rb @@ -0,0 +1,183 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' +require_relative '../../../../rubocop/cop/gitlab/rescue_standard_error_without_tracking' + +RSpec.describe RuboCop::Cop::Gitlab::RescueStandardErrorWithoutTracking, feature_category: :shared do + let(:msg) do + 'Avoid rescuing StandardError without logging or tracking. ' \ + 'Use Gitlab::ErrorTracking.track_and_raise_for_dev or add logging, ' \ + 'or rescue specific error types instead.' + end + + it 'registers an offense for rescuing StandardError without tracking' do + expect_offense(<<~RUBY) + begin + do_something + rescue StandardError => e + ^^^^^^ #{msg} + return nil + end + RUBY + end + + it 'registers an offense for implicit StandardError rescue without tracking' do + expect_offense(<<~RUBY) + begin + do_something + rescue => e + ^^^^^^ #{msg} + return nil + end + RUBY + end + + it 'does not register an offense when using Gitlab::ErrorTracking.track_and_raise_for_dev' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + Gitlab::ErrorTracking.track_and_raise_for_dev(e) + return nil + end + RUBY + end + + it 'does not register an offense when logging the error' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + logger.error("Something went wrong: #{e.message}") + return nil + end + RUBY + end + + it 'does not register an offense when using Rails.logger' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + Rails.logger.error("Something went wrong: #{e.message}") + return nil + end + RUBY + end + + it 'does not register an offense when re-raising the error' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + cleanup_resources + raise + end + RUBY + end + + it 'does not register an offense when raising a different error' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + raise CustomError, "Custom message" + end + RUBY + end + + it 'does not register an offense for specific error types' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue ActiveRecord::RecordNotFound => e + return nil + end + RUBY + end + + it 'does not register an offense for multiple specific error types' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue ActiveRecord::RecordNotFound, ActiveRecord::RecordInvalid => e + return nil + end + RUBY + end + + it 'registers an offense when StandardError is mixed with specific types' do + expect_offense(<<~RUBY) + begin + do_something + rescue ActiveRecord::RecordNotFound, StandardError => e + ^^^^^^ #{msg} + return nil + end + RUBY + end + + it 'does not register an offense when error tracking is in a conditional' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + if some_condition + Gitlab::ErrorTracking.track_and_raise_for_dev(e) + end + return nil + end + RUBY + end + + it 'does not register an offense when logging is in a block' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + with_context do + logger.error("Error occurred: #{e.message}") + end + return nil + end + RUBY + end + + it 'does not register an offense when error tracking is in a case statement' do + expect_no_offenses(<<~RUBY) + begin + do_something + rescue StandardError => e + case e.class.name + when 'SomeError' + handle_some_error + else + Gitlab::ErrorTracking.track_and_raise_for_dev(e) + end + return nil + end + RUBY + end + + it 'registers an offense when rescue block is empty' do + expect_offense(<<~RUBY) + begin + do_something + rescue StandardError => e + ^^^^^^ #{msg} + end + RUBY + end + + it 'registers an offense when rescue block only has non-logging statements' do + expect_offense(<<~RUBY) + begin + do_something + rescue StandardError => e + ^^^^^^ #{msg} + cleanup_resources + return false + end + RUBY + end +end