diff --git a/CHANGELOG.md b/CHANGELOG.md index 34ca9aea..56e1240d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,14 @@ Changelog | [#700](https://github.com/bugsnag/bugsnag-ruby/pull/700) * Add `Configuration#endpoints` for reading the notify and sessions endpoints and `Configuration#endpoints=` for setting them | [#701](https://github.com/bugsnag/bugsnag-ruby/pull/701) +* Add `Configuration#redacted_keys`. This is like `meta_data_filters` but matches strings with case-insensitive equality, rather than matching based on inclusion + | [#703](https://github.com/bugsnag/bugsnag-ruby/pull/703) ### Deprecated * In the next major release, `params` will only contain query string parameters. Currently it also contains the request body for form data requests, but this is deprecated in favour of the new `body` property * The `Configuration#set_endpoints` method is now deprecated in favour of `Configuration#endpoints=` +* The `Configuration#meta_data_filters` option is now deprecated in favour of `Configuration#redacted_keys` ## v6.23.0 (21 September 2021) diff --git a/features/fixtures/docker-compose.yml b/features/fixtures/docker-compose.yml index 016a4ca2..84e2c09c 100644 --- a/features/fixtures/docker-compose.yml +++ b/features/fixtures/docker-compose.yml @@ -35,6 +35,7 @@ services: - BUGSNAG_PROXY_PASSWORD - BUGSNAG_PROXY_PORT - BUGSNAG_PROXY_USER + - BUGSNAG_REDACTED_KEYS - BUGSNAG_RELEASE_STAGE - BUGSNAG_SEND_CODE - BUGSNAG_SEND_ENVIRONMENT diff --git a/features/fixtures/plain/app/app.rb b/features/fixtures/plain/app/app.rb index a0c3b338..2af88b89 100644 --- a/features/fixtures/plain/app/app.rb +++ b/features/fixtures/plain/app/app.rb @@ -22,6 +22,7 @@ def configure_using_environment conf.proxy_password = ENV["BUGSNAG_PROXY_PASSWORD"] if ENV.include? "BUGSNAG_PROXY_PASSWORD" conf.proxy_port = ENV["BUGSNAG_PROXY_PORT"] if ENV.include? "BUGSNAG_PROXY_PORT" conf.proxy_user = ENV["BUGSNAG_PROXY_USER"] if ENV.include? "BUGSNAG_PROXY_USER" + conf.redacted_keys << ENV["BUGSNAG_REDACTED_KEYS"] if ENV.include? "BUGSNAG_REDACTED_KEYS" conf.send_environment = ENV["BUGSNAG_SEND_ENVIRONMENT"] != "false" conf.send_code = ENV["BUGSNAG_SEND_CODE"] != "false" conf.timeout = ENV["BUGSNAG_TIMEOUT"] if ENV.include? "BUGSNAG_TIMEOUT" diff --git a/features/plain_features/filters.feature b/features/plain_features/filters.feature index 2c023404..bedf87fa 100644 --- a/features/plain_features/filters.feature +++ b/features/plain_features/filters.feature @@ -12,3 +12,10 @@ Scenario: Additional filters can be added to the filter list 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 "metaData.filter.filter_me" equals "[FILTERED]" + +Scenario: Redacted keys can also be used to filter sensitive data + Given I set environment variable "BUGSNAG_REDACTED_KEYS" to "filter_me" + When I run the service "plain-ruby" with the command "bundle exec ruby filters/additional_filters.rb" + 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 "metaData.filter.filter_me" equals "[FILTERED]" diff --git a/lib/bugsnag/cleaner.rb b/lib/bugsnag/cleaner.rb index b33d0660..75b7383c 100644 --- a/lib/bugsnag/cleaner.rb +++ b/lib/bugsnag/cleaner.rb @@ -25,7 +25,7 @@ def clean_object(object) # @param url [String] # @return [String] def clean_url(url) - return url if @configuration.meta_data_filters.empty? + return url if @configuration.meta_data_filters.empty? && @configuration.redacted_keys.empty? uri = URI(url) return url unless uri.query @@ -43,6 +43,33 @@ def clean_url(url) uri.to_s end + ## + # @param key [String, #to_s] + # @return [Boolean] + def filters_match?(key) + str = key.to_s + + matched = @configuration.meta_data_filters.any? do |filter| + case filter + when Regexp + str.match(filter) + else + str.include?(filter.to_s) + end + end + + return true if matched + + @configuration.redacted_keys.any? do |redaction_pattern| + case redaction_pattern + when Regexp + str.match(redaction_pattern) + when String + str.downcase == redaction_pattern.downcase + end + end + end + private ## @@ -54,9 +81,11 @@ def clean_url(url) # # @return [Boolean] def deep_filters? - @configuration.meta_data_filters.any? do |filter| + is_deep_filter = proc do |filter| filter.is_a?(Regexp) && filter.to_s.include?("\\.".freeze) end + + @configuration.meta_data_filters.any?(&is_deep_filter) || @configuration.redacted_keys.any?(&is_deep_filter) end def clean_string(str) @@ -137,22 +166,6 @@ def traverse_object(obj, seen, scope) value end - ## - # @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.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 diff --git a/lib/bugsnag/configuration.rb b/lib/bugsnag/configuration.rb index ff775dd4..416a65f5 100644 --- a/lib/bugsnag/configuration.rb +++ b/lib/bugsnag/configuration.rb @@ -56,9 +56,20 @@ class Configuration # A list of keys that should be filtered out from the report and breadcrumb # metadata before sending them to Bugsnag + # @deprecated Use {#redacted_keys} instead # @return [Set] attr_accessor :meta_data_filters + # A set of keys that should be redacted from the report and breadcrumb + # metadata before sending them to Bugsnag + # + # When adding strings, keys that are equal to the string (ignoring case) + # will be redacted. When adding regular expressions, any keys which match + # the regular expression will be redacted + # + # @return [Set] + attr_accessor :redacted_keys + # The logger to use for Bugsnag log messages # @return [Logger] attr_accessor :logger @@ -201,6 +212,7 @@ def initialize self.send_environment = false self.send_code = true self.meta_data_filters = Set.new(DEFAULT_META_DATA_FILTERS) + @redacted_keys = Set.new self.scopes_to_filter = DEFAULT_SCOPES_TO_FILTER self.hostname = default_hostname self.runtime_versions = {} diff --git a/lib/bugsnag/middleware/rack_request.rb b/lib/bugsnag/middleware/rack_request.rb index a9de2aca..387e5df4 100644 --- a/lib/bugsnag/middleware/rack_request.rb +++ b/lib/bugsnag/middleware/rack_request.rb @@ -138,7 +138,7 @@ def parsed_request_body(request, env) end def add_cookies(report, request) - return unless record_cookies?(report.configuration) + return unless record_cookies? cookies = request.cookies rescue nil @@ -147,17 +147,10 @@ def add_cookies(report, request) report.add_metadata(:request, :cookies, cookies) end - def record_cookies?(configuration) + def record_cookies? # only record cookies in the request if none of the filters match "Cookie" # the "Cookie" header will be filtered as normal - configuration.meta_data_filters.none? do |filter| - case filter - when Regexp - COOKIE_HEADER.match(filter) - else - COOKIE_HEADER.include?(filter.to_s) - end - end + !Bugsnag.cleaner.filters_match?(COOKIE_HEADER) end end end diff --git a/spec/cleaner_spec.rb b/spec/cleaner_spec.rb index 96bb9dfe..df33c449 100644 --- a/spec/cleaner_spec.rb +++ b/spec/cleaner_spec.rb @@ -276,6 +276,200 @@ def id expect(cleaner.clean_object(params)).to eq({ request: { params: { foo: { bar: 'baz' } } } }) end + describe "redacted keys" do + it "redacts exact string match" do + object = { events: { metaData: { foo: 'bar', bar: 'foo' } } } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set['foo'] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ + events: { + metaData: { + foo: '[FILTERED]', + bar: 'foo' + } + } + }) + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set['bar'] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ + events: { + metaData: { + foo: 'bar', + bar: '[FILTERED]' + } + } + }) + end + + it "redacts case insensitive string match" do + object = { events: { metaData: { foo: 'bar', bar: 'foo' } } } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set['FOO'] + + cleaner = Bugsnag::Cleaner.new(configuration) + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ + events: { + metaData: { + foo: '[FILTERED]', + bar: 'foo' + } + } + }) + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set['bAr'] + + cleaner = Bugsnag::Cleaner.new(configuration) + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ + events: { + metaData: { + foo: 'bar', + bar: '[FILTERED]' + } + } + }) + end + + it "redacts by regular expression" do + object = { events: { metaData: { foo: 'bar', bar: 'foo' } } } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/fb?/] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ + events: { + metaData: { + foo: '[FILTERED]', + bar: 'foo' + } + } + }) + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/fb+/] + + cleaner = Bugsnag::Cleaner.new(configuration) + expect(cleaner.clean_object(object)).to eq({ + events: { + metaData: { + foo: 'bar', + bar: 'foo' + } + } + }) + end + + it "redacts deeply nested keys" do + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + params = { events: { metaData: { foo: { bar: 'baz', baz: 'bar' } } } } + expect(cleaner.clean_object(params)).to eq({ + events: { + metaData: { + foo: { + bar: '[FILTERED]', + baz: 'bar' + } + } + } + }) + end + + it "redacts deeply nested request parameters" do + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + params = { events: { metaData: { request: { params: { foo: { bar: 'baz', baz: 'bar' } } } } } } + expect(cleaner.clean_object(params)).to eq({ + events: { + metaData: { + request: { + params: { + foo: { + bar: '[FILTERED]', + baz: 'bar' + } + } + } + } + } + }) + end + + it "doesn't filter by string match when the scope is not in 'scopes_to_filter'" do + object = { foo: 'bar', bar: 'foo' } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set['foo'] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar', bar: 'foo' }) + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set['bar'] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar', bar: 'foo' }) + end + + it "doesn't filter by regular expression when the scope is not in 'scopes_to_filter'" do + object = { foo: 'bar', bar: 'foo' } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/fb?/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar', bar: 'foo' }) + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/fb+/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(object)).to eq({ foo: 'bar', bar: 'foo' }) + end + + it "doesn't filter deeply nested keys when the scope is not in 'scopes_to_filter'" do + params = { foo: { bar: 'baz', baz: 'bar' } } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(params)).to eq({ foo: { bar: 'baz', baz: 'bar' } }) + 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', baz: 'bar' } } } } + + configuration = Bugsnag::Configuration.new + configuration.redacted_keys = Set[/^foo\.bar/] + + cleaner = Bugsnag::Cleaner.new(configuration) + + expect(cleaner.clean_object(params)).to eq({ request: { params: { foo: { bar: 'baz', baz: 'bar' } } } }) + end + end + it "filters objects which can't be stringified" do class StringRaiser def to_s diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 30767728..58c3104b 100644 --- a/spec/integrations/rack_spec.rb +++ b/spec/integrations/rack_spec.rb @@ -152,6 +152,63 @@ class Request expect(report.metadata[:session]).to eq({ session: true }) end + it "correctly redacts from url and referer any value indicated by redacted_keys" do + rack_env = { + env: true, + HTTP_REFERER: "https://bugsnag.com/about?email=hello@world.com&another_param=thing", + "rack.session" => { session: true } + } + + rack_request = double + allow(rack_request).to receive_messages( + params: { param: 'test', param2: 'test2' }, + ip: "rack_ip", + request_method: "TEST", + path: "/TEST_PATH", + scheme: "http", + host: "test_host", + port: 80, + referer: "https://bugsnag.com/about?email=hello@world.com&another_param=thing", + fullpath: "/TEST_PATH?email=hello@world.com&another_param=thing", + form_data?: true, + POST: { param: 'test' }, + cookies: { session_id: 12345 } + ) + + expect(::Rack::Request).to receive(:new).with(rack_env).and_return(rack_request) + + Bugsnag.configure do |config| + config.send_environment = true + config.meta_data_filters = [] + config.redacted_keys = Set["email", "cookie"] + config.request_data[:rack_env] = rack_env + end + + report = Bugsnag::Report.new(RuntimeError.new('abc'), Bugsnag.configuration) + + callback = double + expect(callback).to receive(:call).with(report) + + middleware = Bugsnag::Middleware::RackRequest.new(callback) + middleware.call(report) + + expect(report.request).to eq({ + url: "http://test_host/TEST_PATH?email=[FILTERED]&another_param=thing", + httpMethod: "TEST", + params: { param: 'test', param2: 'test2' }, + referer: "https://bugsnag.com/about?email=[FILTERED]&another_param=thing", + clientIp: "rack_ip", + headers: { + "Referer" => "https://bugsnag.com/about?email=[FILTERED]&another_param=thing" + }, + body: { param: 'test' } + }) + + expect(report.metadata[:request]).to be(report.request) + expect(report.metadata[:environment]).to eq(rack_env) + expect(report.metadata[:session]).to eq({ session: true }) + end + it "correctly extracts data from rack middleware" do rack_env = { env: true, diff --git a/spec/report_spec.rb b/spec/report_spec.rb index 3f25877b..7ab5dea7 100644 --- a/spec/report_spec.rb +++ b/spec/report_spec.rb @@ -1155,6 +1155,62 @@ def gloops } end + it "filters params from all payload hashes if they are added to redacted_keys as a string" do + Bugsnag.configuration.redacted_keys << "other_data" + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_metadata(:request, { + params: { + password: "1234", + other_password: "123456", + other_data: "123456", + more_other_data: "123456", + abc: "xyz" + } + }) + 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"]["request"]).not_to be_nil + expect(event["metaData"]["request"]["params"]).not_to be_nil + expect(event["metaData"]["request"]["params"]["password"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["other_password"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["other_data"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["more_other_data"]).to eq("123456") + expect(event["metaData"]["request"]["params"]["abc"]).to eq("xyz") + } + end + + it "filters params from all payload hashes if they are added to redacted_keys as partial regex" do + Bugsnag.configuration.redacted_keys << /r_data/ + + Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| + report.add_metadata(:request, { + params: { + password: "1234", + other_password: "123456", + other_data: "123456", + more_other_data: "123456", + abc: "xyz" + } + }) + 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"]["request"]).not_to be_nil + expect(event["metaData"]["request"]["params"]).not_to be_nil + expect(event["metaData"]["request"]["params"]["password"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["other_password"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["other_data"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["more_other_data"]).to eq("[FILTERED]") + expect(event["metaData"]["request"]["params"]["abc"]).to eq("xyz") + } + end + it "does not notify if report ignored" do Bugsnag.notify(BugsnagTestException.new("It crashed")) do |report| report.ignore! @@ -1861,6 +1917,31 @@ def to_s } end + it "redacted keys apply to breadcrumb metadata" do + Bugsnag.leave_breadcrumb("Test breadcrumb", { + :forbidden_key => false, + :allowed_key => true + }) + + Bugsnag.configuration.redacted_keys << "forbidden_key" + + notify_test_exception + + expect(Bugsnag).to have_sent_notification { |payload, headers| + event = get_event_from_payload(payload) + expect(event["breadcrumbs"].size).to eq(1) + expect(event["breadcrumbs"].first).to match({ + "name" => "Test breadcrumb", + "type" => "manual", + "metaData" => { + "forbidden_key" => "[FILTERED]", + "allowed_key" => true + }, + "timestamp" => match(timestamp_regex) + }) + } + end + it "defaults to an empty array" do notify_test_exception expect(Bugsnag).to have_sent_notification { |payload, headers|