diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 1328252ed0..bd0c3b20ef 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,10 @@ +* `ActiveSupport::ErrorReporter` now accepts and forward a `source:` parameter. + + This allow libraries to signal the origin of the errors, and reporters + to easily ignore some sources. + + *Jean Boussier* + * Fix and add protections for XSS in `ActionView::Helpers` and `ERB::Util`. Add the method `ERB::Util.xml_name_escape` to escape dangerous characters diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index 29d0cee056..006a69f9c4 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -300,8 +300,12 @@ module ActiveSupport def rescue_error_with(fallback) yield rescue Dalli::DalliError => error - ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning) logger.error("DalliError (#{error}): #{error.message}") if logger + ActiveSupport.error_reporter&.report( + error, + severity: :warning, + source: "mem_cache_store.active_support", + ) fallback end end diff --git a/activesupport/lib/active_support/cache/redis_cache_store.rb b/activesupport/lib/active_support/cache/redis_cache_store.rb index dfc95c5d09..a3a88ffa7d 100644 --- a/activesupport/lib/active_support/cache/redis_cache_store.rb +++ b/activesupport/lib/active_support/cache/redis_cache_store.rb @@ -60,6 +60,11 @@ module ActiveSupport if logger logger.error { "RedisCacheStore: #{method} failed, returned #{returning.inspect}: #{exception.class}: #{exception.message}" } end + ActiveSupport.error_reporter&.report( + exception, + severity: :warning, + source: "redis_cache_store.active_support", + ) end # The maximum number of entries to receive per SCAN call. @@ -460,7 +465,6 @@ module ActiveSupport def failsafe(method, returning: nil) yield rescue ::Redis::BaseError => error - ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning) @error_handler&.call(method: method, exception: error, returning: returning) returning end diff --git a/activesupport/lib/active_support/error_reporter.rb b/activesupport/lib/active_support/error_reporter.rb index 25ff0f51cb..52034b44a5 100644 --- a/activesupport/lib/active_support/error_reporter.rb +++ b/activesupport/lib/active_support/error_reporter.rb @@ -40,6 +40,7 @@ module ActiveSupport # end class ErrorReporter SEVERITIES = %i(error warning info) + DEFAULT_SOURCE = "application" attr_accessor :logger @@ -54,17 +55,17 @@ module ActiveSupport # 1 + '1' # end # - def handle(error_class = StandardError, severity: :warning, context: {}, fallback: nil) + def handle(error_class = StandardError, severity: :warning, context: {}, fallback: nil, source: DEFAULT_SOURCE) yield rescue error_class => error - report(error, handled: true, severity: severity, context: context) + report(error, handled: true, severity: severity, context: context, source: source) fallback.call if fallback end - def record(error_class = StandardError, severity: :error, context: {}) + def record(error_class = StandardError, severity: :error, context: {}, source: DEFAULT_SOURCE) yield rescue error_class => error - report(error, handled: false, severity: severity, context: context) + report(error, handled: false, severity: severity, context: context, source: source) raise end @@ -107,7 +108,7 @@ module ActiveSupport # When the block based +handle+ and +record+ methods are not suitable, you can directly use +report+ # # Rails.error.report(error) - def report(error, handled: true, severity: handled ? :warning : :error, context: {}) + def report(error, handled: true, severity: handled ? :warning : :error, context: {}, source: DEFAULT_SOURCE) unless SEVERITIES.include?(severity) raise ArgumentError, "severity must be one of #{SEVERITIES.map(&:inspect).join(", ")}, got: #{severity.inspect}" end @@ -116,7 +117,7 @@ module ActiveSupport disabled_subscribers = ActiveSupport::IsolatedExecutionState[self] @subscribers.each do |subscriber| unless disabled_subscribers&.any? { |s| s === subscriber } - subscriber.report(error, handled: handled, severity: severity, context: full_context) + subscriber.report(error, handled: handled, severity: severity, context: full_context, source: source) end rescue => subscriber_error if logger diff --git a/activesupport/lib/active_support/execution_wrapper.rb b/activesupport/lib/active_support/execution_wrapper.rb index 5a4a9b2c60..38b8cad634 100644 --- a/activesupport/lib/active_support/execution_wrapper.rb +++ b/activesupport/lib/active_support/execution_wrapper.rb @@ -91,7 +91,7 @@ module ActiveSupport begin yield rescue => error - error_reporter.report(error, handled: false) + error_reporter.report(error, handled: false, source: "unhandled_error.active_support") raise ensure instance.complete! diff --git a/activesupport/test/error_reporter_test.rb b/activesupport/test/error_reporter_test.rb index 4423db86a4..daf49215fc 100644 --- a/activesupport/test/error_reporter_test.rb +++ b/activesupport/test/error_reporter_test.rb @@ -15,8 +15,8 @@ class ErrorReporterTest < ActiveSupport::TestCase @events = [] end - def report(error, handled:, severity:, context:) - @events << [error, handled, severity, context] + def report(error, handled:, severity:, source:, context:) + @events << [error, handled, severity, source, context] end end @@ -31,14 +31,20 @@ class ErrorReporterTest < ActiveSupport::TestCase @reporter.set_context(section: "admin") error = ArgumentError.new("Oops") @reporter.report(error, handled: true) - assert_equal [[error, true, :warning, { section: "admin" }]], @subscriber.events + assert_equal [[error, true, :warning, "application", { section: "admin" }]], @subscriber.events end test "passed context has priority over the execution context" do @reporter.set_context(section: "admin") error = ArgumentError.new("Oops") @reporter.report(error, handled: true, context: { section: "public" }) - assert_equal [[error, true, :warning, { section: "public" }]], @subscriber.events + assert_equal [[error, true, :warning, "application", { section: "public" }]], @subscriber.events + end + + test "passed source is forwarded" do + error = ArgumentError.new("Oops") + @reporter.report(error, handled: true, source: "my_gem") + assert_equal [[error, true, :warning, "my_gem", {}]], @subscriber.events end test "#disable allow to skip a subscriber" do @@ -60,7 +66,7 @@ class ErrorReporterTest < ActiveSupport::TestCase @reporter.handle do raise error end - assert_equal [[error, true, :warning, {}]], @subscriber.events + assert_equal [[error, true, :warning, "application", {}]], @subscriber.events end test "#handle can be scoped to an exception class" do @@ -117,7 +123,7 @@ class ErrorReporterTest < ActiveSupport::TestCase raise error end end - assert_equal [[error, false, :error, {}]], @subscriber.events + assert_equal [[error, false, :error, "application", {}]], @subscriber.events end test "#record can be scoped to an exception class" do @@ -164,7 +170,7 @@ class ErrorReporterTest < ActiveSupport::TestCase @error = error end - def report(_error, handled:, severity:, context:) + def report(_error, handled:, severity:, context:, source:) raise @error end end diff --git a/activesupport/test/executor_test.rb b/activesupport/test/executor_test.rb index 893a7c98a7..bcdf28348e 100644 --- a/activesupport/test/executor_test.rb +++ b/activesupport/test/executor_test.rb @@ -13,8 +13,8 @@ class ExecutorTest < ActiveSupport::TestCase @events = [] end - def report(error, handled:, severity:, context:) - @events << [error, handled, severity, context] + def report(error, handled:, severity:, source:, context:) + @events << [error, handled, severity, source, context] end end @@ -27,7 +27,7 @@ class ExecutorTest < ActiveSupport::TestCase raise error end end - assert_equal [[error, false, :error, {}]], subscriber.events + assert_equal [[error, false, :error, "unhandled_error.active_support", {}]], subscriber.events end def test_wrap_invokes_callbacks