diff --git a/sentry-rails/spec/active_job/support/harness.rb b/sentry-rails/spec/active_job/support/harness.rb index 81764eee1..ca4ed21f5 100644 --- a/sentry-rails/spec/active_job/support/harness.rb +++ b/sentry-rails/spec/active_job/support/harness.rb @@ -61,7 +61,7 @@ def perform(*_args, **_kwargs) config.dsn = "http://12345:67890@sentry.localdomain:3000/sentry/42" config.transport.transport_class = Sentry::DummyTransport config.background_worker_threads = 0 - config.include_local_variables = true + config.data_collection.stack_frame_variables = true configure_sentry.call(config, ::Rails.application) if configure_sentry end diff --git a/sentry-rails/spec/support/test_helper.rb b/sentry-rails/spec/support/test_helper.rb index b5397c889..a04ea0ce7 100644 --- a/sentry-rails/spec/support/test_helper.rb +++ b/sentry-rails/spec/support/test_helper.rb @@ -20,7 +20,7 @@ def make_basic_app(&block) # For sending events synchronously config.background_worker_threads = 0 - config.include_local_variables = true + config.data_collection.stack_frame_variables = true yield(config, app) if block_given? end diff --git a/sentry-ruby/lib/sentry-ruby.rb b/sentry-ruby/lib/sentry-ruby.rb index 499497732..b63fe067e 100644 --- a/sentry-ruby/lib/sentry-ruby.rb +++ b/sentry-ruby/lib/sentry-ruby.rb @@ -277,7 +277,7 @@ def init(&block) @background_worker = Sentry::BackgroundWorker.new(config) @session_flusher = config.session_tracking? ? Sentry::SessionFlusher.new(config, client) : nil @backpressure_monitor = config.enable_backpressure_handling ? Sentry::BackpressureMonitor.new(config, client) : nil - exception_locals_tp.enable if config.include_local_variables + exception_locals_tp.enable if config.data_collection.collect_stack_frame_variables? at_exit { close } end @@ -301,7 +301,7 @@ def close client.configuration.run_after_close_callbacks client.flush - if client.configuration.include_local_variables + if client.configuration.data_collection.collect_stack_frame_variables? exception_locals_tp.disable end end diff --git a/sentry-ruby/lib/sentry/configuration.rb b/sentry-ruby/lib/sentry/configuration.rb index a88e11a35..e5a146d40 100644 --- a/sentry-ruby/lib/sentry/configuration.rb +++ b/sentry-ruby/lib/sentry/configuration.rb @@ -171,7 +171,12 @@ class Configuration # Whether to capture local variables from the raised exception's frame. Default is false. # @deprecated Use {#data_collection} and `stack_frame_variables` instead. # @return [Boolean] - attr_accessor :include_local_variables + attr_reader :include_local_variables + + def include_local_variables=(value) + @include_local_variables = value + @data_collection.backfill_stack_frame_variables(self) + end # Whether to capture events and traces into Spotlight. Default is false. # If you set this to true, Sentry will send events and traces to the local @@ -529,6 +534,7 @@ def build_validation_proc(optional, type) def initialize run_callbacks(:before, :initialize) + self.data_collection = DataCollection.new self.app_dirs_pattern = APP_DIRS_PATTERN self.debug = Sentry::Utils::EnvHelper.env_to_bool(ENV["SENTRY_DEBUG"]) self.background_worker_threads = (processor_count / 2.0).ceil @@ -552,7 +558,6 @@ def initialize self.sample_rate = 1.0 self.send_modules = true - self.data_collection = DataCollection.new self.send_default_pii = false self.skip_rake_integration = false self.send_client_reports = true diff --git a/sentry-ruby/lib/sentry/data_collection.rb b/sentry-ruby/lib/sentry/data_collection.rb index b7198a7f1..e71ae3a64 100644 --- a/sentry-ruby/lib/sentry/data_collection.rb +++ b/sentry-ruby/lib/sentry/data_collection.rb @@ -108,9 +108,10 @@ def initialize(document:, variables:) # @default `true` attr_accessor :queues - # @return [Boolean] - # @default `false` - attr_accessor :stack_frame_variables + # @return [Boolean, KeyValueCollection] + # A boolean is shorthand for `mode: :deny_list` (`true`) or `mode: :off` (`false`). + # @default `false` (Ruby-specific to avoid TracePoint overhead) + attr_reader :stack_frame_variables # @return [Integer] # @default `3` @@ -128,8 +129,8 @@ def self.default_filter # Builds data collection settings compatible with the legacy send_default_pii # configuration. def self.backfill(configuration) - # the new DataCollection defaults are already correct if pii is enabled data_collection = new + data_collection.backfill_stack_frame_variables(configuration) return data_collection if configuration.send_default_pii # TODO-neel-data map to exact ruby behaviour for backwards compat behavior @@ -145,7 +146,6 @@ def self.backfill(configuration) data_collection.graphql.variables = false data_collection.database_query_data = false data_collection.queues = false - data_collection.stack_frame_variables = configuration.include_local_variables data_collection.frame_context_lines = configuration.context_lines data_collection end @@ -162,10 +162,24 @@ def initialize @database_query_data = true @graphql = GraphQL.new(document: true, variables: true) @queues = true - @stack_frame_variables = false + self.stack_frame_variables = false @frame_context_lines = 3 end + # Copies the legacy stack frame variable configuration. + def backfill_stack_frame_variables(configuration) + self.stack_frame_variables = configuration.include_local_variables + end + + # Returns whether stack frame local variables should be captured. + def collect_stack_frame_variables? + stack_frame_variables.mode != :off + end + + def stack_frame_variables=(value) + @stack_frame_variables = KeyValueCollection.from(value) + end + def cookies=(value) @cookies = KeyValueCollection.from(value) end diff --git a/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb b/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb index e747b3c6f..4612cd7ac 100644 --- a/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb +++ b/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb @@ -73,14 +73,14 @@ def initialize(mode:, terms:) def mode=(mode) @mode = case mode when true then :deny_list - when false then :off + when false, nil then :off else mode end end # Converts the boolean shorthand into a collection configuration. def self.from(value) - return new(mode: value, terms: nil) if value.equal?(true) || value.equal?(false) + return new(mode: value, terms: nil) if value.equal?(true) || value.equal?(false) || value.nil? value end diff --git a/sentry-ruby/lib/sentry/error_event.rb b/sentry-ruby/lib/sentry/error_event.rb index a76e8b2ab..f9286a6cb 100644 --- a/sentry-ruby/lib/sentry/error_event.rb +++ b/sentry-ruby/lib/sentry/error_event.rb @@ -32,7 +32,12 @@ def add_exception_interface(exception, mechanism:) @extra.merge!(exception.sentry_context) end - @exception = Sentry::ExceptionInterface.build(exception: exception, stacktrace_builder: @stacktrace_builder, mechanism: mechanism) + @exception = Sentry::ExceptionInterface.build( + exception: exception, + stacktrace_builder: @stacktrace_builder, + mechanism: mechanism, + data_collection: @data_collection + ) end end end diff --git a/sentry-ruby/lib/sentry/interfaces/exception.rb b/sentry-ruby/lib/sentry/interfaces/exception.rb index 4ca18d5ae..79b4210f0 100644 --- a/sentry-ruby/lib/sentry/interfaces/exception.rb +++ b/sentry-ruby/lib/sentry/interfaces/exception.rb @@ -25,15 +25,21 @@ def to_h # @see SingleExceptionInterface#build_with_stacktrace # @see SingleExceptionInterface#initialize # @param mechanism [Mechanism] + # @param data_collection [DataCollection] # @return [ExceptionInterface] - def self.build(exception:, stacktrace_builder:, mechanism:) + def self.build(exception:, stacktrace_builder:, mechanism:, data_collection:) exceptions = Sentry::Utils::ExceptionCauseChain.exception_to_array(exception).reverse processed_backtrace_ids = Set.new exceptions = exceptions.map do |e| if e.backtrace && !processed_backtrace_ids.include?(e.backtrace.object_id) processed_backtrace_ids << e.backtrace.object_id - SingleExceptionInterface.build_with_stacktrace(exception: e, stacktrace_builder: stacktrace_builder, mechanism: mechanism) + SingleExceptionInterface.build_with_stacktrace( + exception: e, + stacktrace_builder: stacktrace_builder, + mechanism: mechanism, + data_collection: data_collection + ) else SingleExceptionInterface.new(exception: exception, mechanism: mechanism) end diff --git a/sentry-ruby/lib/sentry/interfaces/single_exception.rb b/sentry-ruby/lib/sentry/interfaces/single_exception.rb index 883e5946e..0aa17b786 100644 --- a/sentry-ruby/lib/sentry/interfaces/single_exception.rb +++ b/sentry-ruby/lib/sentry/interfaces/single_exception.rb @@ -41,7 +41,7 @@ def to_h # patch this method if you want to change an exception's stacktrace frames # also see `StacktraceBuilder.build`. - def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:) + def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:, data_collection:) stacktrace = stacktrace_builder.build(backtrace: exception.backtrace) if locals = exception.instance_variable_get(:@sentry_locals) @@ -60,7 +60,7 @@ def self.build_with_stacktrace(exception:, stacktrace_builder:, mechanism:) end end - stacktrace.frames.last&.vars = locals + stacktrace.frames.last&.vars = data_collection.stack_frame_variables.filter(locals) end new(exception: exception, stacktrace: stacktrace, mechanism: mechanism) diff --git a/sentry-ruby/lib/sentry/test_helper.rb b/sentry-ruby/lib/sentry/test_helper.rb index 5edae4b13..d3bfbf8b6 100644 --- a/sentry-ruby/lib/sentry/test_helper.rb +++ b/sentry-ruby/lib/sentry/test_helper.rb @@ -33,7 +33,7 @@ def setup_sentry_test(&block) dummy_config.background_worker_threads = 0 # user can overwrite some of the configs, with a few exceptions like: - # - include_local_variables + # - data_collection # - auto_session_tracking block&.call(dummy_config) diff --git a/sentry-ruby/spec/sentry/client_spec.rb b/sentry-ruby/spec/sentry/client_spec.rb index 354716d74..01a8e9ac8 100644 --- a/sentry-ruby/spec/sentry/client_spec.rb +++ b/sentry-ruby/spec/sentry/client_spec.rb @@ -608,8 +608,9 @@ def sentry_context context "if local variable contains illegal/malformed encoding characters" do before do + configuration.data_collection.stack_frame_variables = true perform_basic_setup do |config| - config.include_local_variables = true + config.data_collection.stack_frame_variables = true end end diff --git a/sentry-ruby/spec/sentry/configuration_spec.rb b/sentry-ruby/spec/sentry/configuration_spec.rb index 9942439fb..717bac074 100644 --- a/sentry-ruby/spec/sentry/configuration_spec.rb +++ b/sentry-ruby/spec/sentry/configuration_spec.rb @@ -10,6 +10,49 @@ expect(configuration.data_collection.user_info).to eq(false) end + + it "falls back to include_local_variables for stack frame variables" do + configuration = Sentry::Configuration.new do |config| + config.include_local_variables = true + end + + expect(configuration.data_collection.stack_frame_variables.mode).to eq(:deny_list) + end + + it "disables stack frame variables when include_local_variables is false" do + configuration = Sentry::Configuration.new do |config| + config.include_local_variables = false + end + + expect(configuration.data_collection.stack_frame_variables.mode).to eq(:off) + end + + it "treats nil include_local_variables as disabled" do + configuration = Sentry::Configuration.new do |config| + config.include_local_variables = nil + end + + expect(configuration.data_collection.stack_frame_variables.mode).to eq(:off) + end + + it "only backfills stack frame variables when include_local_variables changes" do + configuration = Sentry::Configuration.new do |config| + config.data_collection.user_info = true + config.include_local_variables = true + end + + expect(configuration.data_collection.user_info).to eq(true) + expect(configuration.data_collection.stack_frame_variables.mode).to eq(:deny_list) + end + + it "preserves include_local_variables when send_default_pii is set later" do + configuration = Sentry::Configuration.new do |config| + config.include_local_variables = true + config.send_default_pii = true + end + + expect(configuration.data_collection.stack_frame_variables.mode).to eq(:deny_list) + end end describe "deprecated configuration warnings" do diff --git a/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb b/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb index 1896feaec..f528c41b3 100644 --- a/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb +++ b/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb @@ -25,6 +25,10 @@ expect(described_class.from(false).mode).to eq(:off) end + it "maps nil to off mode" do + expect(described_class.from(nil).mode).to eq(:off) + end + it "returns an existing collection unchanged" do expect(described_class.from(collection)).to be(collection) end @@ -37,9 +41,11 @@ expect(collection.mode).to eq(:deny_list) end - it "maps false to off mode" do + it "maps false and nil to off mode" do collection.mode = false + expect(collection.mode).to eq(:off) + collection.mode = nil expect(collection.mode).to eq(:off) end end diff --git a/sentry-ruby/spec/sentry/data_collection_spec.rb b/sentry-ruby/spec/sentry/data_collection_spec.rb index 71983ae32..8ddf5a92d 100644 --- a/sentry-ruby/spec/sentry/data_collection_spec.rb +++ b/sentry-ruby/spec/sentry/data_collection_spec.rb @@ -29,7 +29,7 @@ expect(data_collection.graphql.document).to eq(false) expect(data_collection.graphql.variables).to eq(false) expect(data_collection.queues).to eq(false) - expect(data_collection.stack_frame_variables).to eq(false) + expect(data_collection.stack_frame_variables.mode).to eq(:off) expect(data_collection.frame_context_lines).to eq(3) end @@ -85,7 +85,7 @@ expect(data_collection.graphql.document).to eq(true) expect(data_collection.graphql.variables).to eq(true) expect(data_collection.queues).to eq(true) - expect(data_collection.stack_frame_variables).to eq(false) + expect(data_collection.stack_frame_variables.mode).to eq(:off) expect(data_collection.frame_context_lines).to eq(3) end end @@ -116,11 +116,28 @@ data_collection.http_headers.request = false data_collection.http_headers.response = true data_collection.url_query_params = false + data_collection.stack_frame_variables = false expect(data_collection.cookies.mode).to eq(:deny_list) expect(data_collection.http_headers.request.mode).to eq(:off) expect(data_collection.http_headers.response.mode).to eq(:deny_list) expect(data_collection.url_query_params.mode).to eq(:off) + expect(data_collection.stack_frame_variables.mode).to eq(:off) + + data_collection.stack_frame_variables = true + expect(data_collection.stack_frame_variables.mode).to eq(:deny_list) + end + + it "supports configuring stack frame variables by name" do + data_collection.stack_frame_variables.mode = :allow_list + data_collection.stack_frame_variables.terms = ["user"] + + expect(data_collection.stack_frame_variables.mode).to eq(:allow_list) + expect(data_collection.stack_frame_variables.terms).to eq(["user"]) + end + + it "does not collect stack frame variables by default" do + expect(data_collection.collect_stack_frame_variables?).to eq(false) end it "supports configuring request and response headers independently" do diff --git a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb index 734952339..7c87376f4 100644 --- a/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb +++ b/sentry-ruby/spec/sentry/rack/capture_exceptions_spec.rb @@ -93,10 +93,10 @@ expect(env.key?("sentry.error_event_id")).to eq(false) end - context "with config.include_local_variables = true" do + context "with config.data_collection.stack_frame_variables = true" do before do perform_basic_setup do |config| - config.include_local_variables = true + config.data_collection.stack_frame_variables = true end end @@ -121,6 +121,43 @@ expect(last_frame[:vars]).to include({ a: "1", b: "0" }) end + it 'filters locals by name' do + perform_basic_setup do |config| + config.data_collection.stack_frame_variables.mode = :allow_list + config.data_collection.stack_frame_variables.terms = ["safe"] + end + + app = ->(_e) do + safe = "visible" + foo = "bar" + raise "boom" + end + + stack = Sentry::Rack::CaptureExceptions.new(app) + + expect { stack.call(env) }.to raise_error(RuntimeError, "boom") + + event = last_sentry_event.to_h + last_frame = event.dig(:exception, :values, 0, :stacktrace, :frames).last + expect(last_frame[:vars]).to include(safe: "visible", foo: "[Filtered]") + end + + it 'filters locals matching the sensitive denylist by default' do + app = ->(_e) do + safe = "visible" + password = "filtered" + raise "boom" + end + + stack = Sentry::Rack::CaptureExceptions.new(app) + + expect { stack.call(env) }.to raise_error(RuntimeError, "boom") + + event = last_sentry_event.to_h + last_frame = event.dig(:exception, :values, 0, :stacktrace, :frames).last + expect(last_frame[:vars]).to include(safe: "visible", password: "[Filtered]") + end + it 'ignores problematic locals' do class Foo def inspect diff --git a/sentry-ruby/spec/sentry_spec.rb b/sentry-ruby/spec/sentry_spec.rb index 2754f63f1..85cdae3a5 100644 --- a/sentry-ruby/spec/sentry_spec.rb +++ b/sentry-ruby/spec/sentry_spec.rb @@ -391,7 +391,7 @@ end end - context "with include_local_variables = false (default)" do + context "with data_collection.stack_frame_variables = false (default)" do it "doens't capture local variables" do begin 1/0 @@ -405,10 +405,10 @@ end end - context "with include_local_variables = true" do + context "with data_collection.stack_frame_variables = true" do before do perform_basic_setup do |config| - config.include_local_variables = true + config.data_collection.stack_frame_variables = true end end @@ -1575,7 +1575,7 @@ it "disables Tracepoint" do perform_basic_setup do |config| - config.include_local_variables = true + config.data_collection.stack_frame_variables = true end expect(described_class.exception_locals_tp).to receive(:disable).and_call_original diff --git a/spec/apps/rails-mini/app.rb b/spec/apps/rails-mini/app.rb index 48f75988d..be8114d4c 100644 --- a/spec/apps/rails-mini/app.rb +++ b/spec/apps/rails-mini/app.rb @@ -68,7 +68,7 @@ def debug_log_path config.sdk_logger.level = ::Logger::DEBUG config.sdk_logger = Sentry::Logger.new($stdout) config.debug = true - config.include_local_variables = true + config.data_collection.stack_frame_variables = true config.release = "sentry-ruby-rails-mini-#{Time.now.utc}" config.transport.transport_class = Sentry::DebugTransport config.sdk_debug_transport_log_file = debug_log_path.join("sentry_debug_events.log")