diff --git a/.rubocop.yml b/.rubocop.yml index d8da878af..aa916e963 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -24,12 +24,27 @@ Lint/RaiseException: Lint/StructNewOverride: Enabled: true +Lint/DeprecatedOpenSSLConstant: + Enabled: true + +Lint/MixedRegexpCaptureTypes: + Enabled: true + +Style/RedundantFetchBlock: + Enabled: true + Style/ExponentialNotation: Enabled: false Style/HashEachMethods: Enabled: true +Style/RedundantRegexpCharacterClass: + Enabled: true + +Style/RedundantRegexpEscape: + Enabled: true + # These require newer version of Ruby than our minimum supported version, so # have to be disabled Style/HashTransformKeys: # Requires Ruby 2.5 diff --git a/CHANGELOG.md b/CHANGELOG.md index 053bbfeac..08799dd3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,11 @@ Changelog * The Breadcrumb name limit of 30 characters has been removed | [#600](https://github.com/bugsnag/bugsnag-ruby/pull/600) +* Improve performance of payload cleaning + | [#601](https://github.com/bugsnag/bugsnag-ruby/pull/601) + ### Deprecated + * The `ignore_classes` configuration option has been deprecated in favour of `discard_classes`. `ignore_classes` will be removed in the next major release ## 6.13.1 (11 May 2020) diff --git a/TESTING.md b/TESTING.md index f5fe99e87..513d83588 100644 --- a/TESTING.md +++ b/TESTING.md @@ -42,7 +42,7 @@ aws configure --profile=opensource Subsequently you'll need to run the following commmand to authenticate with the registry: ``` -$(aws ecr get-login --profile=opensource --no-include-email) +aws ecr get-login-password --profile=opensource | docker login --username AWS --password-stdin 855461928731.dkr.ecr.us-west-1.amazonaws.com ``` __Your session will periodically expire__, so you'll need to run this command to re-authenticate when that happens. @@ -64,7 +64,7 @@ Configure the tests to be run in the following way: When running the end-to-end tests, you'll want to restrict the feature files run to the specific test features for the platform. This is done using the Cucumber CLI syntax at the end of the `docker-compose run ruby-maze-runner` command, i.e: ``` -RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run ruby-maze-runner features/rails_features --tags "@rails6" +RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run --use-aliases ruby-maze-runner features/rails_features --tags "@rails6" ``` - Plain ruby tests should target `features/plain_features` @@ -75,7 +75,7 @@ RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run ruby-maze-runner featur In order to target specific features the exact `.feature` file can be specified, i.e: ``` -RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run ruby-maze-runner features/rails_features/app_version.feature --tags "@rails6" +RUBY_TEST_VERSION=2.6 RAILS_VERSION=6 docker-compose run --use-aliases ruby-maze-runner features/rails_features/app_version.feature --tags "@rails6" ``` In order to avoid running flakey or unfinished tests, the tag `"not @wip"` can be added to the tags option. This is recommended for all CI runs. If a tag is already specified, this should be added using the `and` keyword, e.g. `--tags "@rails6 and not @wip"` diff --git a/dockerfiles/Dockerfile.ruby-maze-runner b/dockerfiles/Dockerfile.ruby-maze-runner index 3b72c15b8..de09d813c 100644 --- a/dockerfiles/Dockerfile.ruby-maze-runner +++ b/dockerfiles/Dockerfile.ruby-maze-runner @@ -20,7 +20,7 @@ COPY spec ./spec RUN gem build bugsnag.gemspec # The maze-runner node tests -FROM 855461928731.dkr.ecr.us-west-1.amazonaws.com/maze-runner:v2-cli as ruby-maze-runner +FROM 855461928731.dkr.ecr.us-west-1.amazonaws.com/maze-runner:latest-cli as ruby-maze-runner WORKDIR /app/ COPY features ./features COPY --from=ruby-package-builder /app/bugsnag-*.gem bugsnag.gem diff --git a/features/fixtures/rails3/app/config/initializers/bugsnag.rb b/features/fixtures/rails3/app/config/initializers/bugsnag.rb index fea970a97..b849c0995 100644 --- a/features/fixtures/rails3/app/config/initializers/bugsnag.rb +++ b/features/fixtures/rails3/app/config/initializers/bugsnag.rb @@ -11,10 +11,11 @@ config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE" config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false" config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true" + config.meta_data_filters << 'filtered_parameter' if ENV["SQL_ONLY_BREADCRUMBS"] == "true" config.before_breadcrumb_callbacks << Proc.new do |breadcrumb| breadcrumb.ignore! unless breadcrumb.meta_data[:event_name] == "sql.active_record" && breadcrumb.meta_data[:name] == "User Load" end end -end \ No newline at end of file +end diff --git a/features/fixtures/rails4/app/config/initializers/bugsnag.rb b/features/fixtures/rails4/app/config/initializers/bugsnag.rb index d7ce3b9b3..b849c0995 100644 --- a/features/fixtures/rails4/app/config/initializers/bugsnag.rb +++ b/features/fixtures/rails4/app/config/initializers/bugsnag.rb @@ -11,6 +11,7 @@ config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE" config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false" config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true" + config.meta_data_filters << 'filtered_parameter' if ENV["SQL_ONLY_BREADCRUMBS"] == "true" config.before_breadcrumb_callbacks << Proc.new do |breadcrumb| diff --git a/features/fixtures/rails5/app/config/initializers/bugsnag.rb b/features/fixtures/rails5/app/config/initializers/bugsnag.rb index d7ce3b9b3..b849c0995 100644 --- a/features/fixtures/rails5/app/config/initializers/bugsnag.rb +++ b/features/fixtures/rails5/app/config/initializers/bugsnag.rb @@ -11,6 +11,7 @@ config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE" config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false" config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true" + config.meta_data_filters << 'filtered_parameter' if ENV["SQL_ONLY_BREADCRUMBS"] == "true" config.before_breadcrumb_callbacks << Proc.new do |breadcrumb| diff --git a/features/fixtures/rails6/app/config/initializers/bugsnag.rb b/features/fixtures/rails6/app/config/initializers/bugsnag.rb index d7ce3b9b3..b849c0995 100644 --- a/features/fixtures/rails6/app/config/initializers/bugsnag.rb +++ b/features/fixtures/rails6/app/config/initializers/bugsnag.rb @@ -11,6 +11,7 @@ config.release_stage = ENV["BUGSNAG_RELEASE_STAGE"] if ENV.include? "BUGSNAG_RELEASE_STAGE" config.send_code = ENV["BUGSNAG_SEND_CODE"] != "false" config.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] == "true" + config.meta_data_filters << 'filtered_parameter' if ENV["SQL_ONLY_BREADCRUMBS"] == "true" config.before_breadcrumb_callbacks << Proc.new do |breadcrumb| diff --git a/features/rails_features/meta_data_filters.feature b/features/rails_features/meta_data_filters.feature index fc6b4a8f6..b867e63e7 100644 --- a/features/rails_features/meta_data_filters.feature +++ b/features/rails_features/meta_data_filters.feature @@ -3,12 +3,14 @@ Feature: Metadata filters @rails3 @rails4 @rails5 @rails6 Scenario: Meta_data_filters should include Rails.configuration.filter_parameters Given I start the rails service - When I navigate to the route "/metadata_filters/filter" on the rails app + When I navigate to the route "/metadata_filters/filter?filtered_parameter=foo&other_parameter=bar" on the rails app And I wait to receive a request Then the request is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier" And the event "unhandled" is false And the exception "errorClass" equals "RuntimeError" And the exception "message" starts with "handled string" And the event "app.type" equals "rails" - And the event "metaData.request.url" ends with "/metadata_filters/filter" + And the event "metaData.request.url" ends with "/metadata_filters/filter?filtered_parameter=[FILTERED]&other_parameter=bar" And the event "metaData.my_specific_filter" equals "[FILTERED]" + And the event "metaData.request.params.filtered_parameter" equals "[FILTERED]" + And the event "metaData.request.params.other_parameter" equals "bar" diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index 608a6edf6..1be5d084a 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -33,6 +33,7 @@ require "bugsnag/breadcrumbs/breadcrumb" require "bugsnag/breadcrumbs/breadcrumbs" +# rubocop:todo Metrics/ModuleLength module Bugsnag LOCK = Mutex.new INTEGRATIONS = [:resque, :sidekiq, :mailman, :delayed_job, :shoryuken, :que, :mongo] @@ -63,7 +64,7 @@ def notify(exception, auto_notify=false, &block) auto_notify = false end - return unless deliver_notification?(exception, auto_notify) + return unless should_deliver_notification?(exception, auto_notify) exception = NIL_EXCEPTION_DESCRIPTION if exception.nil? @@ -71,6 +72,7 @@ def notify(exception, auto_notify=false, &block) # If this is an auto_notify we yield the block before the any middleware is run yield(report) if block_given? && auto_notify + if report.ignore? configuration.debug("Not notifying #{report.exceptions.last[:errorClass]} due to ignore being signified in auto_notify block") return @@ -97,6 +99,7 @@ def notify(exception, auto_notify=false, &block) # If this is not an auto_notify then the block was provided by the user. This should be the last # block that is run as it is the users "most specific" block. yield(report) if block_given? && !auto_notify + if report.ignore? configuration.debug("Not notifying #{report.exceptions.last[:errorClass]} due to ignore being signified in user provided block") return @@ -111,13 +114,7 @@ def notify(exception, auto_notify=false, &block) report.severity_reason = initial_reason end - # Deliver - configuration.info("Notifying #{configuration.notify_endpoint} of #{report.exceptions.last[:errorClass]}") - options = {:headers => report.headers} - payload = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(report.as_json)) - Bugsnag::Delivery[configuration.delivery_method].deliver(configuration.notify_endpoint, payload, configuration, options) - report_summary = report.summary - leave_breadcrumb(report_summary[:error_class], report_summary, Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, :auto) + deliver_notification(report) end end @@ -212,27 +209,40 @@ def leave_breadcrumb(name, meta_data={}, type=Bugsnag::Breadcrumbs::MANUAL_BREAD validator.validate(breadcrumb) # Skip if it's already invalid - unless breadcrumb.ignore? - # Run callbacks - configuration.before_breadcrumb_callbacks.each do |c| - c.arity > 0 ? c.call(breadcrumb) : c.call - break if breadcrumb.ignore? - end + return if breadcrumb.ignore? - # Return early if ignored - return if breadcrumb.ignore? + # Run callbacks + configuration.before_breadcrumb_callbacks.each do |c| + c.arity > 0 ? c.call(breadcrumb) : c.call + break if breadcrumb.ignore? + end + + # Return early if ignored + return if breadcrumb.ignore? - # Validate again in case of callback alteration - validator.validate(breadcrumb) + # Validate again in case of callback alteration + validator.validate(breadcrumb) - # Add to breadcrumbs buffer if still valid - configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore? + # Add to breadcrumbs buffer if still valid + configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore? + end + + ## + # Returns the client's Cleaner object, or creates one if not yet created. + # + # @api private + # + # @return [Cleaner] + def cleaner + @cleaner = nil unless defined?(@cleaner) + @cleaner || LOCK.synchronize do + @cleaner ||= Bugsnag::Cleaner.new(configuration) end end private - def deliver_notification?(exception, auto_notify) + def should_deliver_notification?(exception, auto_notify) reason = abort_reason(exception, auto_notify) configuration.debug(reason) unless reason.nil? reason.nil? @@ -250,6 +260,32 @@ def abort_reason(exception, auto_notify) end end + ## + # Deliver the notification to Bugsnag + # + # @param report [Report] + # @return void + def deliver_notification(report) + configuration.info("Notifying #{configuration.notify_endpoint} of #{report.exceptions.last[:errorClass]}") + + payload = report_to_json(report) + options = {:headers => report.headers} + + Bugsnag::Delivery[configuration.delivery_method].deliver( + configuration.notify_endpoint, + payload, + configuration, + options + ) + + leave_breadcrumb( + report.summary[:error_class], + report.summary, + Bugsnag::Breadcrumbs::ERROR_BREADCRUMB_TYPE, + :auto + ) + end + # Check if the API key is valid and warn (once) if it is not def check_key_valid @key_warning = false unless defined?(@key_warning) @@ -274,7 +310,23 @@ def check_endpoint_setup raise ArgumentError, "The session endpoint cannot be modified without the notify endpoint" end end + + ## + # Convert the Report object to JSON + # + # We ensure the report is safe to send by removing recursion, fixing + # encoding errors and redacting metadata according to "meta_data_filters" + # + # @param report [Report] + # @return string + def report_to_json(report) + cleaned = cleaner.clean_object(report.as_json) + trimmed = Bugsnag::Helpers.trim_if_needed(cleaned) + + ::JSON.dump(trimmed) + end end end +# rubocop:enable Metrics/ModuleLength Bugsnag.load_integrations unless ENV["BUGSNAG_DISABLE_AUTOCONFIGURE"] diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index ea7539d86..25ca20508 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -1,19 +1,75 @@ require 'uri' module Bugsnag + # @api private class Cleaner FILTERED = '[FILTERED]'.freeze RECURSION = '[RECURSION]'.freeze OBJECT = '[OBJECT]'.freeze RAISED = '[RAISED]'.freeze - def initialize(filters) - @filters = Array(filters) - @deep_filters = @filters.any? {|f| f.kind_of?(Regexp) && f.to_s.include?("\\.".freeze) } + ## + # @param configuration [Configuration] + def initialize(configuration) + @configuration = configuration end - def clean_object(obj) - traverse_object(obj, {}, nil) + def clean_object(object) + @deep_filters = deep_filters? + + traverse_object(object, {}, nil) + end + + ## + # @param url [String] + # @return [String] + def clean_url(url) + return url if @configuration.meta_data_filters.empty? + + uri = URI(url) + return url unless uri.query + + query_params = uri.query.split('&').map { |pair| pair.split('=') } + query_params.map! do |key, val| + if filters_match?(key) + "#{key}=#{FILTERED}" + else + "#{key}=#{val}" + end + end + + uri.query = query_params.join('&') + uri.to_s + end + + private + + ## + # This method calculates whether we need to filter deeply or not; i.e. whether + # we should match both with and without 'request.params' + # + # This is cached on the instance variable '@deep_filters' for performance + # reasons + # + # @return [Boolean] + def deep_filters? + @configuration.meta_data_filters.any? do |filter| + filter.is_a?(Regexp) && filter.to_s.include?("\\.".freeze) + end + end + + def clean_string(str) + if defined?(str.encoding) && defined?(Encoding::UTF_8) + if str.encoding == Encoding::UTF_8 + str.valid_encoding? ? str : str.encode('utf-16', invalid: :replace, undef: :replace).encode('utf-8') + else + str.encode('utf-8', invalid: :replace, undef: :replace) + end + elsif defined?(Iconv) + Iconv.conv('UTF-8//IGNORE', 'UTF-8', str) || str + else + str + end end def traverse_object(obj, seen, scope) @@ -28,12 +84,14 @@ def traverse_object(obj, seen, scope) value = case obj when Hash clean_hash = {} - obj.each do |k,v| + obj.each do |k, v| begin - if filters_match_deeply?(k, scope) + current_scope = [scope, k].compact.join('.') + + if filters_match_deeply?(k, current_scope) clean_hash[k] = FILTERED else - clean_hash[k] = traverse_object(v, seen, [scope, k].compact.join('.')) + clean_hash[k] = traverse_object(v, seen, current_scope) end # If we get an error here, we assume the key needs to be filtered # to avoid leaking things we shouldn't. We also remove the key itself @@ -73,67 +131,56 @@ def traverse_object(obj, seen, scope) value end - def clean_string(str) - if defined?(str.encoding) && defined?(Encoding::UTF_8) - if str.encoding == Encoding::UTF_8 - str.valid_encoding? ? str : str.encode('utf-16', invalid: :replace, undef: :replace).encode('utf-8') + ## + # @param key [String, #to_s] + # @return [Boolean] + def filters_match?(key) + str = key.to_s + + @configuration.meta_data_filters.any? do |filter| + case filter + when Regexp + str.match(filter) else - str.encode('utf-8', invalid: :replace, undef: :replace) + str.include?(filter.to_s) end - elsif defined?(Iconv) - Iconv.conv('UTF-8//IGNORE', 'UTF-8', str) || str - else - str end end - def self.clean_object_encoding(obj) - new(nil).clean_object(obj) - end + ## + # If someone has a Rails filter like /^stuff\.secret/, it won't match + # "request.params.stuff.secret", so we try it both with and without the + # "request.params." bit. + # + # @param key [String, #to_s] + # @param scope [String] + # @return [Boolean] + def filters_match_deeply?(key, scope) + return false unless scope_should_be_filtered?(scope) - def clean_url(url) - return url if @filters.empty? + return true if filters_match?(key) + return false unless @deep_filters - uri = URI(url) - return url unless uri.query + return true if filters_match?(scope) - query_params = uri.query.split('&').map { |pair| pair.split('=') } - query_params.map! do |key, val| - if filters_match?(key) - "#{key}=#{FILTERED}" + @configuration.scopes_to_filter.any? do |scope_to_filter| + if scope.start_with?("#{scope_to_filter}.request.params.") + filters_match?(scope.sub("#{scope_to_filter}.request.params.", '')) else - "#{key}=#{val}" + filters_match?(scope.sub("#{scope_to_filter}.", '')) end end - - uri.query = query_params.join('&') - uri.to_s end - private - - def filters_match?(key) - str = key.to_s - - @filters.any? do |f| - case f - when Regexp - str.match(f) - else - str.include?(f.to_s) - end + ## + # Should the given scope be filtered? + # + # @param scope [String] + # @return [Boolean] + def scope_should_be_filtered?(scope) + @configuration.scopes_to_filter.any? do |scope_to_filter| + scope.start_with?("#{scope_to_filter}.") end end - - # If someone has a Rails filter like /^stuff\.secret/, it won't match "request.params.stuff.secret", - # so we try it both with and without the "request.params." bit. - def filters_match_deeply?(key, scope) - return true if filters_match?(key) - return false unless @deep_filters - - long = [scope, key].compact.join('.') - short = long.sub(/^request\.params\./, '') - filters_match?(long) || filters_match?(short) - end end end diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index 0a8e5aded..d017c111e 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -72,6 +72,10 @@ class Configuration # @return [Regexp] matching file paths out of project attr_accessor :vendor_path + ## + # @return [Array] + attr_reader :scopes_to_filter + API_KEY_REGEX = /[0-9a-f]{32}/i THREAD_LOCAL_NAME = "bugsnag_req_data" @@ -91,7 +95,9 @@ class Configuration DEFAULT_MAX_BREADCRUMBS = 25 # Path to vendored code. Used to mark file paths as out of project. - DEFAULT_VENDOR_PATH = %r{^(vendor\/|\.bundle\/)} + DEFAULT_VENDOR_PATH = %r{^(vendor/|\.bundle/)} + + DEFAULT_SCOPES_TO_FILTER = ['events.metaData', 'events.breadcrumbs.metaData'].freeze alias :track_sessions :auto_capture_sessions alias :track_sessions= :auto_capture_sessions= @@ -104,6 +110,7 @@ def initialize self.send_environment = false self.send_code = true self.meta_data_filters = Set.new(DEFAULT_META_DATA_FILTERS) + self.scopes_to_filter = DEFAULT_SCOPES_TO_FILTER self.hostname = default_hostname self.runtime_versions = {} self.runtime_versions["ruby"] = RUBY_VERSION @@ -313,6 +320,8 @@ def disable_sessions private + attr_writer :scopes_to_filter + PROG_NAME = "[Bugsnag]" def default_hostname diff --git a/lib/bugsnag/helpers.rb b/lib/bugsnag/helpers.rb index 99a3c2f21..21223f74a 100644 --- a/lib/bugsnag/helpers.rb +++ b/lib/bugsnag/helpers.rb @@ -17,12 +17,10 @@ module Helpers def self.trim_if_needed(value) value = "" if value.nil? - # Sanitize object - sanitized_value = Bugsnag::Cleaner.clean_object_encoding(value) - return sanitized_value unless payload_too_long?(sanitized_value) + return value unless payload_too_long?(value) # Trim metadata - reduced_value = trim_metadata(sanitized_value) + reduced_value = trim_metadata(value) return reduced_value unless payload_too_long?(reduced_value) # Trim code from stacktrace diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 3a2a39719..6825ccf21 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -28,18 +28,16 @@ def call(report) url = "#{request.scheme}://#{request.host}" url << ":#{request.port}" unless [80, 443].include?(request.port) - cleaner = Bugsnag::Cleaner.new(report.configuration.meta_data_filters) - # If app is passed a bad URL, this code will crash attempting to clean it begin - url << cleaner.clean_url(request.fullpath) + url << Bugsnag.cleaner.clean_url(request.fullpath) rescue StandardError => stde Bugsnag.configuration.warn "RackRequest - Rescued error while cleaning request.fullpath: #{stde}" end referer = nil begin - referer = cleaner.clean_url(request.referer) if request.referer + referer = Bugsnag.cleaner.clean_url(request.referer) if request.referer rescue StandardError => stde Bugsnag.configuration.warn "RackRequest - Rescued error while cleaning request.referer: #{stde}" end diff --git a/lib/bugsnag/report.rb b/lib/bugsnag/report.rb index c3b1e54aa..66b3ed7fd 100644 --- a/lib/bugsnag/report.rb +++ b/lib/bugsnag/report.rb @@ -97,6 +97,7 @@ def as_json releaseStage: release_stage, type: app_type }, + breadcrumbs: breadcrumbs.map(&:to_h), context: context, device: { hostname: hostname, @@ -104,6 +105,7 @@ def as_json }, exceptions: exceptions, groupingHash: grouping_hash, + metaData: meta_data, session: session, severity: severity, severityReason: severity_reason, @@ -111,19 +113,7 @@ def as_json user: user } - # cleanup character encodings - payload_event = Bugsnag::Cleaner.clean_object_encoding(payload_event) - - # filter out sensitive values in (and cleanup encodings) metaData - filter_cleaner = Bugsnag::Cleaner.new(configuration.meta_data_filters) - payload_event[:metaData] = filter_cleaner.clean_object(meta_data) - payload_event[:breadcrumbs] = breadcrumbs.map do |breadcrumb| - breadcrumb_hash = breadcrumb.to_h - breadcrumb_hash[:metaData] = filter_cleaner.clean_object(breadcrumb_hash[:metaData]) - breadcrumb_hash - end - - payload_event.reject! {|k,v| v.nil? } + payload_event.reject! {|k, v| v.nil? } # return the payload hash { diff --git a/lib/bugsnag/stacktrace.rb b/lib/bugsnag/stacktrace.rb index 1eb0259e0..c147935df 100644 --- a/lib/bugsnag/stacktrace.rb +++ b/lib/bugsnag/stacktrace.rb @@ -9,6 +9,8 @@ class Stacktrace ## # Process a backtrace and the configuration into a parsed stacktrace. + # + # rubocop:todo Metrics/CyclomaticComplexity def initialize(backtrace, configuration) @configuration = configuration @@ -64,6 +66,7 @@ def initialize(backtrace, configuration) end end.compact end + # rubocop:enable Metrics/CyclomaticComplexity ## # Returns the processed backtrace diff --git a/spec/cleaner_spec.rb b/spec/cleaner_spec.rb index b40c657f2..3f207a0e7 100644 --- a/spec/cleaner_spec.rb +++ b/spec/cleaner_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Bugsnag::Cleaner do - subject { described_class.new(nil) } + subject { Bugsnag::Cleaner.new(Bugsnag::Configuration.new) } describe "#clean_object" do is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' @@ -156,23 +156,113 @@ def to_s end it "filters by string inclusion" do - expect(described_class.new(['f']).clean_object({ :foo => 'bar' })).to eq({ :foo => '[FILTERED]' }) - expect(described_class.new(['b']).clean_object({ :foo => 'bar' })).to eq({ :foo => 'bar' }) + object = { events: { metaData: { foo: 'bar' } } } + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = ['f'] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ events: { metaData: { foo: '[FILTERED]' } } }) + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = ['b'] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ events: { metaData: { foo: 'bar' } } }) end it "filters by regular expression" do - expect(described_class.new([/fb?/]).clean_object({ :foo => 'bar' })).to eq({ :foo => '[FILTERED]' }) - expect(described_class.new([/fb+/]).clean_object({ :foo => 'bar' })).to eq({ :foo => 'bar' }) + object = { events: { metaData: { foo: 'bar' } } } + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/fb?/] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ events: { metaData: { foo: '[FILTERED]' } } }) + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/fb+/] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ events: { metaData: { foo: 'bar' } } }) end it "filters deeply nested keys" do - params = {:foo => {:bar => "baz"}} - expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:foo => {:bar => '[FILTERED]'}}) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + params = { events: { metaData: { foo: { bar: 'baz' } } } } + expect(cleaner.clean_object(params)).to eq({ events: { metaData: { foo: { bar: '[FILTERED]' } } } }) end it "filters deeply nested request parameters" do - params = {:request => {:params => {:foo => {:bar => "baz"}}}} - expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:request => {:params => {:foo => {:bar => '[FILTERED]'}}}}) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + params = { events: { metaData: { request: { params: { foo: { bar: 'baz' } } } } } } + expect(cleaner.clean_object(params)).to eq({ events: { metaData: { request: { params: { foo: { bar: '[FILTERED]' } } } } } }) + end + + it "doesn't filter by string inclusion when the scope is not in 'scopes_to_filter'" do + object = { foo: 'bar' } + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = ['f'] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = ['b'] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + end + + it "doesn't filter by regular expression when the scope is not in 'scopes_to_filter'" do + object = { foo: 'bar' } + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/fb?/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/fb+/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + end + + it "doesn't filter deeply nested keys when the scope is not in 'scopes_to_filter'" do + params = { foo: { bar: 'baz' } } + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(params)).to eq({ foo: { bar: 'baz' } }) + end + + it "doesn't filter deeply nested request parameters when the scope is not in 'scopes_to_filter'" do + params = { request: { params: { foo: { bar: 'baz' } } } } + + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(params)).to eq({ request: { params: { foo: { bar: 'baz' } } } }) end it "filters objects which can't be stringified" do @@ -187,7 +277,12 @@ def to_s describe "#clean_url" do let(:filters) { [] } - subject { described_class.new(filters).clean_url(url) } + + subject do + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = filters + described_class.new(configuration).clean_url(url) + end context "with no filters configured" do let(:url) { "/dir/page?param1=value1¶m2=value2" } diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index 57089afbe..11323c932 100644 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -4,23 +4,7 @@ require 'set' describe Bugsnag::Helpers do - describe "trim_if_needed" do - - it "breaks recursion" do - a = [1, 2, 3] - b = [2, a] - a << b - value = Bugsnag::Helpers.trim_if_needed(a) - expect(value).to eq([1, 2, 3, [2, "[RECURSION]"]]) - end - - it "does not break equal objects without recursion" do - data = [1, [1, 2], [1, 2], "a"] - value = Bugsnag::Helpers.trim_if_needed(data) - expect(value).to eq data - end - it "preserves bool types" do value = Bugsnag::Helpers.trim_if_needed([1, 3, true, "NO", "2", false]) expect(value[2]).to be_a(TrueClass) @@ -39,76 +23,7 @@ expect(value[4]).to be_a(String) end - context "an object will throw if `to_s` is called" do - class StringRaiser - def to_s - raise 'Oh no you do not!' - end - end - - it "uses the string '[RAISED]' instead" do - value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRaiser.new]) - expect(value[2]).to eq "[RAISED]" - end - - it "replaces hash key with '[RAISED]'" do - a = {} - a[StringRaiser.new] = 1 - - value = Bugsnag::Helpers.trim_if_needed(a) - expect(value).to eq({ "[RAISED]" => "[FILTERED]" }) - end - - it "uses a single '[RAISED]'key when multiple keys raise" do - a = {} - a[StringRaiser.new] = 1 - a[StringRaiser.new] = 2 - - value = Bugsnag::Helpers.trim_if_needed(a) - expect(value).to eq({ "[RAISED]" => "[FILTERED]" }) - end - end - - context "an object will infinitely recurse if `to_s` is called" do - is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' - - class StringRecurser - def to_s - to_s - end - end - - it "uses the string '[RECURSION]' instead" do - skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby - - value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRecurser.new]) - expect(value[2]).to eq "[RECURSION]" - end - - it "replaces hash key with '[RECURSION]'" do - skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby - - a = {} - a[StringRecurser.new] = 1 - - value = Bugsnag::Helpers.trim_if_needed(a) - expect(value).to eq({ "[RECURSION]" => "[FILTERED]" }) - end - - it "uses a single '[RECURSION]'key when multiple keys recurse" do - skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby - - a = {} - a[StringRecurser.new] = 1 - a[StringRecurser.new] = 2 - - value = Bugsnag::Helpers.trim_if_needed(a) - expect(value).to eq({ "[RECURSION]" => "[FILTERED]" }) - end - end - context "payload length is less than allowed" do - it "does not change strings" do value = SecureRandom.hex(4096) expect(Bugsnag::Helpers.trim_if_needed(value)).to eq value @@ -126,7 +41,6 @@ def to_s end context "payload length is greater than allowed" do - it "trims metadata strings" do payload = { :events => [{ diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 95cc9f013..57847efa4 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -127,9 +127,10 @@ class Request expect(report).to receive(:context=).with("TEST /TEST_PATH") expect(report).to receive(:user).and_return({}) - config = double - allow(config).to receive(:send_environment).and_return(true) - allow(config).to receive(:meta_data_filters).and_return(['email']) + config = Bugsnag.configuration + config.send_environment = true + config.meta_data_filters = ['email'] + allow(report).to receive(:configuration).and_return(config) expect(report).to receive(:add_tab).once.with(:request, { :url => "http://test_host/TEST_PATH?email=[FILTERED]&another_param=thing", @@ -187,9 +188,10 @@ class Request expect(report).to receive(:context=).with("TEST /TEST_PATH") expect(report).to receive(:user).and_return({}) - config = double - allow(config).to receive(:send_environment).and_return(true) - allow(config).to receive(:meta_data_filters).and_return(nil) + config = Bugsnag.configuration + config.send_environment = true + config.meta_data_filters = [] + allow(report).to receive(:configuration).and_return(config) expect(report).to receive(:add_tab).once.with(:environment, rack_env) expect(report).to receive(:add_tab).once.with(:request, { diff --git a/spec/report_spec.rb b/spec/report_spec.rb index 701b985bf..91126274d 100644 --- a/spec/report_spec.rb +++ b/spec/report_spec.rb @@ -1,5 +1,5 @@ # encoding: utf-8 -require 'spec_helper' +require_relative './spec_helper' require 'securerandom' require 'ostruct' @@ -592,7 +592,6 @@ def gloops end it "filters params from all payload hashes if they are set in default meta_data_filters" do - Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| report.meta_data.merge!({ :request => { @@ -638,7 +637,6 @@ def gloops end it "filters params from all payload hashes if they are added to meta_data_filters" do - Bugsnag.configuration.meta_data_filters << "other_data" Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| report.meta_data.merge!({:request => {:params => {:password => "1234", :other_password => "123456", :other_data => "123456"}}}) @@ -656,7 +654,6 @@ def gloops end it "filters params from all payload hashes if they are added to meta_data_filters as regex" do - Bugsnag.configuration.meta_data_filters << /other_data/ Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| report.meta_data.merge!({:request => {:params => {:password => "1234", :other_password => "123456", :other_data => "123456"}}}) @@ -674,7 +671,6 @@ def gloops end it "filters params from all payload hashes if they are added to meta_data_filters as partial regex" do - Bugsnag.configuration.meta_data_filters << /r_data/ Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| report.meta_data.merge!({:request => {:params => {:password => "1234", :other_password => "123456", :other_data => "123456"}}}) @@ -705,6 +701,33 @@ def gloops } end + it "does not apply filters outside of report.meta_data" do + Bugsnag.configuration.meta_data_filters << "data" + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.meta_data = { + xyz: "abc", + data: "123456" + } + + report.user = { + id: 123, + data: "hello" + } + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + + expect(event["metaData"]).not_to be_nil + expect(event["metaData"]["xyz"]).to eq("abc") + expect(event["metaData"]["data"]).to eq("[FILTERED]") + + expect(event["user"]).not_to be_nil + expect(event["user"]["data"]).to eq("hello") + } + end + it "does not notify if report ignored" do Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| report.ignore! @@ -1108,6 +1131,161 @@ def gloops } end + it "should handle recursive metadata" do + a = [1, 2, 3] + b = [2, a] + a << b + c = [1, 2, 3] + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + a: a, + b: b, + c: c + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "a" => [1, 2, 3, [2, "[RECURSION]"]], + "b" => [2, "[RECURSION]"], + "c" => [1, 2, 3] + }) + } + end + + it "does not detect two equal objects as recursion" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + data: [1, [1, 2], [1, 2], "a"] + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "data" => [1, [1, 2], [1, 2], "a"] + }) + } + end + + context "an object that throws if `to_s` is called" do + class StringRaiser + def to_s + raise 'Oh no you do not!' + end + end + + it "uses the string '[RAISED]' instead" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + data: [1, 2, StringRaiser.new] + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "data" => [1, 2, "[RAISED]"] + }) + } + end + + it "replaces hash key with '[RAISED]'" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + StringRaiser.new => 1 + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "[RAISED]" => "[FILTERED]" + }) + } + end + + it "uses a single '[RAISED]'key when multiple keys raise" do + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + StringRaiser.new => 1, + StringRaiser.new => 2 + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "[RAISED]" => "[FILTERED]" + }) + } + end + end + + context "an object that infinitely recurse if `to_s` is called" do + is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' + + class StringRecurser + def to_s + to_s + end + end + + it "uses the string '[RECURSION]' instead" do + skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + data: [1, 2, StringRecurser.new] + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "data" => [1, 2, "[RECURSION]"] + }) + } + end + + it "replaces hash key with '[RECURSION]'" do + skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + StringRecurser.new => 1 + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "[RECURSION]" => "[FILTERED]" + }) + } + end + + it "uses a single '[RECURSION]'key when multiple keys recurse" do + skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_tab(:some_tab, { + StringRecurser.new => 1, + StringRecurser.new => 2 + }) + end + + expect(Bugsnag).to have_sent_notification{ |payload, headers| + event = get_event_from_payload(payload) + expect(event["metaData"]["some_tab"]).to eq({ + "[RECURSION]" => "[FILTERED]" + }) + } + end + end + it 'should handle exceptions with empty backtrace' do begin err = RuntimeError.new diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6584fd1ea..b172d183b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -10,6 +10,7 @@ require 'bugsnag' +require 'tmpdir' require 'webmock/rspec' require 'rspec/expectations' require 'rspec/mocks' @@ -43,12 +44,16 @@ def ruby_version_greater_equal?(version) RSpec.configure do |config| config.order = "random" + config.example_status_persistence_file_path = "#{Dir.tmpdir}/rspec_status" config.before(:each) do WebMock.stub_request(:post, "https://notify.bugsnag.com/") WebMock.stub_request(:post, "https://sessions.bugsnag.com/") Bugsnag.instance_variable_set(:@configuration, Bugsnag::Configuration.new) + Bugsnag.instance_variable_set(:@session_tracker, Bugsnag::SessionTracker.new) + Bugsnag.instance_variable_set(:@cleaner, Bugsnag::Cleaner.new(Bugsnag.configuration)) + Bugsnag.configure do |bugsnag| bugsnag.api_key = "c9d60ae4c7e70c4b6c4ebd3e8056d2b8" bugsnag.release_stage = "production" @@ -83,4 +88,4 @@ def have_sent_notification(&matcher) raise "no matcher provided to have_sent_notification (did you use { })" end end -end \ No newline at end of file +end