From 9f404dc7ed6b44a6f7298bd2bebecc670d77103f Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 23 Jun 2020 17:53:30 +0100 Subject: [PATCH 01/13] Improve performance of cleaning objects This is a first iteration of improving how we clean objects when preparing to JSON encode them Currently we iterate over the payload multiple times; sometimes to clean up encoding errors/recursion and other times to filter sensitive data Ideally we should be iterating over the payload once, which is slightly complicated because we should only be filtering parts of the payload (the metadata and breadcrumb metadata) --- lib/bugsnag.rb | 9 ++++++++- lib/bugsnag/cleaner.rb | 7 +++++++ lib/bugsnag/helpers.rb | 6 ++---- lib/bugsnag/report.rb | 16 +++------------- spec/cleaner_spec.rb | 2 ++ spec/helper_spec.rb | 9 +++++++-- spec/report_spec.rb | 31 +++++++++++++++++++++++++++---- spec/spec_helper.rb | 2 +- 8 files changed, 57 insertions(+), 25 deletions(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index 608a6edf6..f78d67a02 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -114,7 +114,14 @@ def notify(exception, auto_notify=false, &block) # 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)) + + cleaner = Cleaner.new(configuration.meta_data_filters) + + cleaned = cleaner.clean_object(report.as_json) + trimmed = Bugsnag::Helpers.trim_if_needed(cleaned) + + payload = ::JSON.dump(trimmed) + 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) diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index ea7539d86..737b129ea 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -128,6 +128,13 @@ def filters_match?(key) # 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) + # FIXME: This is a hack! + # We don't want to apply filters to places outside of 'events.metaData' + # and 'events.breadcrumbs.metaData' as then we could redact things + # like our stacktraces, which is bad. We should implement this in a + # better way, but this makes the tests pass + return false unless scope.nil? || scope.start_with?('events.metaData') || scope.start_with?('events.breadcrumbs.metaData') + return true if filters_match?(key) return false unless @deep_filters 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/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/spec/cleaner_spec.rb b/spec/cleaner_spec.rb index b40c657f2..3cc0ff0a3 100644 --- a/spec/cleaner_spec.rb +++ b/spec/cleaner_spec.rb @@ -166,11 +166,13 @@ def to_s end it "filters deeply nested keys" do + skip "this will not work until we implement scopes to filter" params = {:foo => {:bar => "baz"}} expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:foo => {:bar => '[FILTERED]'}}) end it "filters deeply nested request parameters" do + skip "this will not work until we implement scopes to filter" params = {:request => {:params => {:foo => {:bar => "baz"}}}} expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:request => {:params => {:foo => {:bar => '[FILTERED]'}}}}) end diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index 57089afbe..e59acaa19 100644 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -8,6 +8,7 @@ describe "trim_if_needed" do it "breaks recursion" do + skip "TODO reimplement elsewhere (report_spec?)" a = [1, 2, 3] b = [2, a] a << b @@ -47,11 +48,13 @@ def to_s end it "uses the string '[RAISED]' instead" do + skip "TODO reimplement elsewhere (report_spec?)" value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRaiser.new]) expect(value[2]).to eq "[RAISED]" end it "replaces hash key with '[RAISED]'" do + skip "TODO reimplement elsewhere (report_spec?)" a = {} a[StringRaiser.new] = 1 @@ -60,6 +63,7 @@ def to_s end it "uses a single '[RAISED]'key when multiple keys raise" do + skip "TODO reimplement elsewhere (report_spec?)" a = {} a[StringRaiser.new] = 1 a[StringRaiser.new] = 2 @@ -79,6 +83,7 @@ def to_s end it "uses the string '[RECURSION]' instead" do + skip "TODO reimplement elsewhere (report_spec?)" skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRecurser.new]) @@ -86,6 +91,7 @@ def to_s end it "replaces hash key with '[RECURSION]'" do + skip "TODO reimplement elsewhere (report_spec?)" skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby a = {} @@ -96,6 +102,7 @@ def to_s end it "uses a single '[RECURSION]'key when multiple keys recurse" do + skip "TODO reimplement elsewhere (report_spec?)" skip "JRuby doesn't allow recovery from SystemStackErrors" if is_jruby a = {} @@ -108,7 +115,6 @@ def to_s 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 +132,6 @@ def to_s end context "payload length is greater than allowed" do - it "trims metadata strings" do payload = { :events => [{ diff --git a/spec/report_spec.rb b/spec/report_spec.rb index 701b985bf..90488abac 100644 --- a/spec/report_spec.rb +++ b/spec/report_spec.rb @@ -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! diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6584fd1ea..2f6061dab 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -83,4 +83,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 From 3888357fe6f55105c4925ac36950bd7cbc333e93 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 10:46:22 +0100 Subject: [PATCH 02/13] Move some tests to report_spec Helper no longer breaks recursion in 'trim_if_needed', so these tests no longer apply there. However we are still breaking recursion and so can test the same thing elsewhere --- spec/helper_spec.rb | 91 ------------------------- spec/report_spec.rb | 157 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 156 insertions(+), 92 deletions(-) diff --git a/spec/helper_spec.rb b/spec/helper_spec.rb index e59acaa19..11323c932 100644 --- a/spec/helper_spec.rb +++ b/spec/helper_spec.rb @@ -4,24 +4,7 @@ require 'set' describe Bugsnag::Helpers do - describe "trim_if_needed" do - - it "breaks recursion" do - skip "TODO reimplement elsewhere (report_spec?)" - 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) @@ -40,80 +23,6 @@ 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 - skip "TODO reimplement elsewhere (report_spec?)" - value = Bugsnag::Helpers.trim_if_needed([1, 3, StringRaiser.new]) - expect(value[2]).to eq "[RAISED]" - end - - it "replaces hash key with '[RAISED]'" do - skip "TODO reimplement elsewhere (report_spec?)" - 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 - skip "TODO reimplement elsewhere (report_spec?)" - 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 "TODO reimplement elsewhere (report_spec?)" - 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 "TODO reimplement elsewhere (report_spec?)" - 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 "TODO reimplement elsewhere (report_spec?)" - 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) diff --git a/spec/report_spec.rb b/spec/report_spec.rb index 90488abac..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' @@ -1131,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 From 6907d598887ed59dd98f0d2a0e3516d8f5727e39 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 10:47:03 +0100 Subject: [PATCH 03/13] Tidy report delivery --- lib/bugsnag.rb | 62 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index f78d67a02..456ef3ec3 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -63,7 +63,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 +71,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 +98,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,20 +113,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} - - cleaner = Cleaner.new(configuration.meta_data_filters) - - cleaned = cleaner.clean_object(report.as_json) - trimmed = Bugsnag::Helpers.trim_if_needed(cleaned) - - payload = ::JSON.dump(trimmed) - - 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 @@ -257,6 +246,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) @@ -281,6 +296,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) + cleaner = Cleaner.new(configuration.meta_data_filters) + + cleaned = cleaner.clean_object(report.as_json) + trimmed = Bugsnag::Helpers.trim_if_needed(cleaned) + + ::JSON.dump(trimmed) + end end end From 4d5f7b53657e4794652b19f11618c8322659ffb9 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 10:47:15 +0100 Subject: [PATCH 04/13] Tidy nesting in leave_breadcrumb --- lib/bugsnag.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index 456ef3ec3..cb02edf6f 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -208,27 +208,27 @@ 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 - # Validate again in case of callback alteration - validator.validate(breadcrumb) + # Return early if ignored + return if breadcrumb.ignore? - # Add to breadcrumbs buffer if still valid - configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore? - end + # Validate again in case of callback alteration + validator.validate(breadcrumb) + + # Add to breadcrumbs buffer if still valid + configuration.breadcrumbs << breadcrumb unless breadcrumb.ignore? 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? From 3275fe6a493625cc89395cb41df15f1567897d5a Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 11:50:31 +0100 Subject: [PATCH 05/13] Provide Cleaner with scopes that it should filter For example, in this hash: { a: { b: 'c' } } 'c' lives in scope 'a.b' and so should only be filtered if Cleaner is given 'a.b' in its 'scopes_to_filter' --- lib/bugsnag.rb | 6 ++- lib/bugsnag/cleaner.rb | 45 +++++++++++----- lib/bugsnag/middleware/rack_request.rb | 2 +- spec/cleaner_spec.rb | 72 +++++++++++++++++++++----- spec/integrations/rack_spec.rb | 2 +- 5 files changed, 97 insertions(+), 30 deletions(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index cb02edf6f..c7eb51539 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -38,6 +38,7 @@ module Bugsnag INTEGRATIONS = [:resque, :sidekiq, :mailman, :delayed_job, :shoryuken, :que, :mongo] NIL_EXCEPTION_DESCRIPTION = "'nil' was notified as an exception" + SCOPES_TO_FILTER = ['events.metaData', 'events.breadcrumbs.metaData'].freeze class << self ## @@ -306,7 +307,10 @@ def check_endpoint_setup # @param report [Report] # @return string def report_to_json(report) - cleaner = Cleaner.new(configuration.meta_data_filters) + cleaner = Cleaner.new( + configuration.meta_data_filters, + SCOPES_TO_FILTER + ) cleaned = cleaner.clean_object(report.as_json) trimmed = Bugsnag::Helpers.trim_if_needed(cleaned) diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index 737b129ea..f65bfa749 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -7,9 +7,13 @@ class Cleaner OBJECT = '[OBJECT]'.freeze RAISED = '[RAISED]'.freeze - def initialize(filters) + ## + # @param filters [Set] + # @param scopes_to_filter [Array] + def initialize(filters, scopes_to_filter) @filters = Array(filters) @deep_filters = @filters.any? {|f| f.kind_of?(Regexp) && f.to_s.include?("\\.".freeze) } + @scopes_to_filter = scopes_to_filter end def clean_object(obj) @@ -28,12 +32,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 @@ -88,7 +94,7 @@ def clean_string(str) end def self.clean_object_encoding(obj) - new(nil).clean_object(obj) + new(Set.new, []).clean_object(obj) end def clean_url(url) @@ -112,6 +118,9 @@ def clean_url(url) private + ## + # @param key [String, #to_s] + # @return [Boolean] def filters_match?(key) str = key.to_s @@ -125,22 +134,30 @@ def filters_match?(key) 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. + # + # @param key [String, #to_s] + # @param scope [String] + # @return [Boolean] def filters_match_deeply?(key, scope) - # FIXME: This is a hack! - # We don't want to apply filters to places outside of 'events.metaData' - # and 'events.breadcrumbs.metaData' as then we could redact things - # like our stacktraces, which is bad. We should implement this in a - # better way, but this makes the tests pass - return false unless scope.nil? || scope.start_with?('events.metaData') || scope.start_with?('events.breadcrumbs.metaData') + return false unless scope_should_be_filtered?(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) + short = scope.sub(/^request\.params\./, '') + filters_match?(scope) || filters_match?(short) + end + + ## + # Should the given scope be filtered according to our 'scopes_to_filter'? + # + # @param scope [String] + # @return [Boolean] + def scope_should_be_filtered?(scope) + @scopes_to_filter.any? {|scope_to_filter| scope.start_with?(scope_to_filter) } end end end diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 3a2a39719..1f1694eb7 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -28,7 +28,7 @@ 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) + 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 diff --git a/spec/cleaner_spec.rb b/spec/cleaner_spec.rb index 3cc0ff0a3..4732059f7 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 { described_class.new(Set.new, []) } describe "#clean_object" do is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' @@ -156,25 +156,71 @@ 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 = { foo: 'bar' } + + cleaner = Bugsnag::Cleaner.new(Set.new(['f']), ['foo']) + expect(cleaner.clean_object(object)).to eq({ foo: '[FILTERED]' }) + + cleaner = Bugsnag::Cleaner.new(Set.new(['b']), ['foo']) + expect(cleaner.clean_object(object)).to eq({ 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 = { foo: 'bar' } + + cleaner = Bugsnag::Cleaner.new(Set.new([/fb?/]), ['foo']) + expect(cleaner.clean_object(object)).to eq({ foo: '[FILTERED]' }) + + cleaner = Bugsnag::Cleaner.new(Set.new([/fb+/]), ['foo']) + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) end it "filters deeply nested keys" do - skip "this will not work until we implement scopes to filter" - params = {:foo => {:bar => "baz"}} - expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:foo => {:bar => '[FILTERED]'}}) + params = { foo: { bar: 'baz' } } + cleaner = Bugsnag::Cleaner.new(Set.new([/^foo\.bar/]), ['foo']) + + expect(cleaner.clean_object(params)).to eq({ foo: { bar: '[FILTERED]' } }) end it "filters deeply nested request parameters" do - skip "this will not work until we implement scopes to filter" - params = {:request => {:params => {:foo => {:bar => "baz"}}}} - expect(described_class.new([/^foo\.bar/]).clean_object(params)).to eq({:request => {:params => {:foo => {:bar => '[FILTERED]'}}}}) + params = { request: { params: { foo: { bar: 'baz' } } } } + cleaner = Bugsnag::Cleaner.new(Set.new([/^foo\.bar/]), ['request']) + + expect(cleaner.clean_object(params)).to eq({ 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' } + + cleaner = Bugsnag::Cleaner.new(Set.new(['f']), ['bar']) + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + + cleaner = Bugsnag::Cleaner.new(Set.new(['b']), ['bar']) + 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' } + + cleaner = Bugsnag::Cleaner.new(Set.new([/fb?/]), ['bar']) + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + + cleaner = Bugsnag::Cleaner.new(Set.new([/fb+/]), ['bar']) + 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' } } + cleaner = Bugsnag::Cleaner.new(Set.new([/^foo\.bar/]), ['baz']) + + 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' } } } } + cleaner = Bugsnag::Cleaner.new(Set.new([/^foo\.bar/]), ['baz']) + + expect(cleaner.clean_object(params)).to eq({ request: { params: { foo: { bar: 'baz' } } } }) end it "filters objects which can't be stringified" do @@ -188,8 +234,8 @@ def to_s end describe "#clean_url" do - let(:filters) { [] } - subject { described_class.new(filters).clean_url(url) } + let(:filters) { Set.new } + subject { described_class.new(filters, []).clean_url(url) } context "with no filters configured" do let(:url) { "/dir/page?param1=value1¶m2=value2" } diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 95cc9f013..025dcb269 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -189,7 +189,7 @@ class Request config = double allow(config).to receive(:send_environment).and_return(true) - allow(config).to receive(:meta_data_filters).and_return(nil) + allow(config).to receive(:meta_data_filters).and_return(Set.new) 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, { From 5762b38b2ea982362efade31635ca94af922a13c Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 12:03:33 +0100 Subject: [PATCH 06/13] Bump Maze Runner to latest --- dockerfiles/Dockerfile.ruby-maze-runner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 7d9b8c62efcaec4d732ce8f3b515bf5ab4adf09d Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 13:30:03 +0100 Subject: [PATCH 07/13] Add pending rubocop cops --- .rubocop.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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 From 0699f905dd145977b8d7a7cc6f990f4d5b68f8a7 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 13:30:33 +0100 Subject: [PATCH 08/13] Add todos for rubocop violations --- lib/bugsnag.rb | 2 ++ lib/bugsnag/stacktrace.rb | 3 +++ 2 files changed, 5 insertions(+) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index c7eb51539..ccdbb5c31 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] @@ -319,5 +320,6 @@ def report_to_json(report) end end end +# rubocop:enable Metrics/ModuleLength Bugsnag.load_integrations unless ENV["BUGSNAG_DISABLE_AUTOCONFIGURE"] 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 From 1203022dc3819a29cd4940fff7c02508befcd752 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 13:30:50 +0100 Subject: [PATCH 09/13] Fix unnecessary regex escapes --- lib/bugsnag/configuration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index 0a8e5aded..bb6209967 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -91,7 +91,7 @@ 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/)} alias :track_sessions :auto_capture_sessions alias :track_sessions= :auto_capture_sessions= From e87761b7c16bc40a3fa9e8e9f0f4e5402b9009d9 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 24 Jun 2020 15:11:23 +0100 Subject: [PATCH 10/13] Add changelog entry for #601 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) 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) From 86d7ce1e05dc98081a0b1581b8054cfc2d07eef9 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Mon, 6 Jul 2020 16:39:57 +0100 Subject: [PATCH 11/13] Remove unused method --- lib/bugsnag/cleaner.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index f65bfa749..01b887e7d 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -93,10 +93,6 @@ def clean_string(str) end end - def self.clean_object_encoding(obj) - new(Set.new, []).clean_object(obj) - end - def clean_url(url) return url if @filters.empty? From 8c669ae107acbac15fa04450610dcbbd884f3f40 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 7 Jul 2020 11:59:35 +0100 Subject: [PATCH 12/13] Make Cleaner a shared instance This exposed a pretty big regression where filters wouldn't match when they should have. This was caused by us filtering the entire report object in one go, which means the scopes were nested deeper than they were before Previously we filtered the events.metaData directly, so scopes would not include 'events.metaData' and therefore a filter of 'foo' would match 'events.metaData.foo'. Now that we filter the entire report, if a filter relied on 'deep_filters', it would apply and so things that should be redacted wouldn't have been To solve this, we strip each 'scope_to_filter' from the scope before matching it, if deep_filters are enabled The tests passed before this change because we set 'scopes_to_filter' in each test. Now that the instance is shared, the scopes are fetched from the Configuration so this isn't possible and it exposed the bug The tests now cover this case, because they can't set 'scopes_to_filter' directly anymore, so they are testing that the filters they're using match with the Configuration's 'scopes_to_filter' --- lib/bugsnag.rb | 19 ++-- lib/bugsnag/cleaner.rb | 133 +++++++++++++++---------- lib/bugsnag/configuration.rb | 9 ++ lib/bugsnag/middleware/rack_request.rb | 6 +- spec/cleaner_spec.rb | 97 +++++++++++++----- spec/integrations/rack_spec.rb | 14 +-- spec/spec_helper.rb | 5 + 7 files changed, 189 insertions(+), 94 deletions(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index ccdbb5c31..1be5d084a 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -39,7 +39,6 @@ module Bugsnag INTEGRATIONS = [:resque, :sidekiq, :mailman, :delayed_job, :shoryuken, :que, :mongo] NIL_EXCEPTION_DESCRIPTION = "'nil' was notified as an exception" - SCOPES_TO_FILTER = ['events.metaData', 'events.breadcrumbs.metaData'].freeze class << self ## @@ -228,6 +227,19 @@ def leave_breadcrumb(name, meta_data={}, type=Bugsnag::Breadcrumbs::MANUAL_BREAD 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 should_deliver_notification?(exception, auto_notify) @@ -308,11 +320,6 @@ def check_endpoint_setup # @param report [Report] # @return string def report_to_json(report) - cleaner = Cleaner.new( - configuration.meta_data_filters, - SCOPES_TO_FILTER - ) - cleaned = cleaner.clean_object(report.as_json) trimmed = Bugsnag::Helpers.trim_if_needed(cleaned) diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index 01b887e7d..25ca20508 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -1,6 +1,7 @@ require 'uri' module Bugsnag + # @api private class Cleaner FILTERED = '[FILTERED]'.freeze RECURSION = '[RECURSION]'.freeze @@ -8,16 +9,67 @@ class Cleaner RAISED = '[RAISED]'.freeze ## - # @param filters [Set] - # @param scopes_to_filter [Array] - def initialize(filters, scopes_to_filter) - @filters = Array(filters) - @deep_filters = @filters.any? {|f| f.kind_of?(Regexp) && f.to_s.include?("\\.".freeze) } - @scopes_to_filter = scopes_to_filter + # @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) @@ -79,60 +131,26 @@ 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') - 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 clean_url(url) - return url if @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 - ## # @param key [String, #to_s] # @return [Boolean] def filters_match?(key) str = key.to_s - @filters.any? do |f| - case f + @configuration.meta_data_filters.any? do |filter| + case filter when Regexp - str.match(f) + str.match(filter) else - str.include?(f.to_s) + str.include?(filter.to_s) end 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. + # 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] @@ -143,17 +161,26 @@ def filters_match_deeply?(key, scope) return true if filters_match?(key) return false unless @deep_filters - short = scope.sub(/^request\.params\./, '') - filters_match?(scope) || filters_match?(short) + return true if filters_match?(scope) + + @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 + filters_match?(scope.sub("#{scope_to_filter}.", '')) + end + end end ## - # Should the given scope be filtered according to our 'scopes_to_filter'? + # Should the given scope be filtered? # # @param scope [String] # @return [Boolean] def scope_should_be_filtered?(scope) - @scopes_to_filter.any? {|scope_to_filter| scope.start_with?(scope_to_filter) } + @configuration.scopes_to_filter.any? do |scope_to_filter| + scope.start_with?("#{scope_to_filter}.") + end end end end diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index bb6209967..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" @@ -93,6 +97,8 @@ class Configuration # Path to vendored code. Used to mark file paths as out of project. 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/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index 1f1694eb7..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/spec/cleaner_spec.rb b/spec/cleaner_spec.rb index 4732059f7..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(Set.new, []) } + subject { Bugsnag::Cleaner.new(Bugsnag::Configuration.new) } describe "#clean_object" do is_jruby = defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' @@ -156,69 +156,111 @@ def to_s end it "filters by string inclusion" do - object = { foo: 'bar' } + object = { events: { metaData: { foo: 'bar' } } } - cleaner = Bugsnag::Cleaner.new(Set.new(['f']), ['foo']) - expect(cleaner.clean_object(object)).to eq({ foo: '[FILTERED]' }) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = ['f'] - cleaner = Bugsnag::Cleaner.new(Set.new(['b']), ['foo']) - expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + 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 - object = { foo: 'bar' } + object = { events: { metaData: { foo: 'bar' } } } - cleaner = Bugsnag::Cleaner.new(Set.new([/fb?/]), ['foo']) - expect(cleaner.clean_object(object)).to eq({ foo: '[FILTERED]' }) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/fb?/] - cleaner = Bugsnag::Cleaner.new(Set.new([/fb+/]), ['foo']) - expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) + 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' } } - cleaner = Bugsnag::Cleaner.new(Set.new([/^foo\.bar/]), ['foo']) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) - expect(cleaner.clean_object(params)).to eq({ foo: { bar: '[FILTERED]' } }) + 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' } } } } - cleaner = Bugsnag::Cleaner.new(Set.new([/^foo\.bar/]), ['request']) + 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: '[FILTERED]' } } } }) + 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' } - cleaner = Bugsnag::Cleaner.new(Set.new(['f']), ['bar']) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = ['f'] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) - cleaner = Bugsnag::Cleaner.new(Set.new(['b']), ['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' } - cleaner = Bugsnag::Cleaner.new(Set.new([/fb?/]), ['bar']) + configuration = Bugsnag::Configuration.new + configuration.meta_data_filters = [/fb?/] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ foo: 'bar' }) - cleaner = Bugsnag::Cleaner.new(Set.new([/fb+/]), ['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' } } - cleaner = Bugsnag::Cleaner.new(Set.new([/^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' } } } } - cleaner = Bugsnag::Cleaner.new(Set.new([/^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 @@ -234,8 +276,13 @@ def to_s end describe "#clean_url" do - let(:filters) { Set.new } - subject { described_class.new(filters, []).clean_url(url) } + let(:filters) { [] } + + 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/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 025dcb269..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(Set.new) + 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/spec_helper.rb b/spec/spec_helper.rb index 2f6061dab..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" From 5700938edf083afb119cec99c1c0292cde9e4221 Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Tue, 7 Jul 2020 14:09:46 +0100 Subject: [PATCH 13/13] Add a Maze Runner test for 'deep filtering' --- TESTING.md | 6 +++--- features/fixtures/rails3/app/config/initializers/bugsnag.rb | 3 ++- features/fixtures/rails4/app/config/initializers/bugsnag.rb | 1 + features/fixtures/rails5/app/config/initializers/bugsnag.rb | 1 + features/fixtures/rails6/app/config/initializers/bugsnag.rb | 1 + features/rails_features/meta_data_filters.feature | 6 ++++-- 6 files changed, 12 insertions(+), 6 deletions(-) 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/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"