getsentry / sentry-ruby

Sentry SDK for Ruby
https://sentry.io/for/ruby
MIT License
933 stars 494 forks source link

Issue scrubbing content in exception when updating ruby version from 3.2.2 to 3.3.0 #2210

Closed aliuk2012 closed 10 months ago

aliuk2012 commented 10 months ago

Issue Description

Hi I've recently tried to update ruby versions across all my teams projects and each projects spec failed for exactly the same reason. It seems that rescuing an exception in 3.3.0 has changed compared to 3.2.0.

Reproduction Steps

I've created a stand-alone rails app with a pull request showing the failing spec as a PR https://github.com/Laings-Online-Limited/sentry-ruby-3.3-bug/pull/3 and as you will see the only thing I changed was the ruby version for it to fail.

Expected Behavior

Expected my tests to pass and scrub out email addresses so they are not logged into Sentry

Actual Behavior

The following spec fails https://github.com/Laings-Online-Limited/sentry-ruby-3.3-bug/blob/main/spec/integration/sentry_spec.rb#L34-L54 when it expects the exception value to be scrubbed.

 1) config/initializers/sentry when an exception is raised containing personally identifying information replaces the email address in the exception with a comment
     Failure/Error: expect(filtered_event.to_hash[:exception][:values].first[:value]).to include "[Filtered (client-side)]"

       expected "undefined method `not_a_method' for an instance of SupportForm (NoMethodError)\n\n      form.not_a_method\n          ^^^^^^^^^^^^^" to include "[Filtered (client-side)]"
       Diff:
       @@ -1,4 +1,7 @@
       -[Filtered (client-side)]
       +undefined method `not_a_method' for an instance of SupportForm (NoMethodError)
       +
       +      form.not_a_method
       +          ^^^^^^^^^^^^^

     # ./spec/integration/sentry_spec.rb:48:in `block (3 levels) in <top (required)>'

Finished in 0.3928 seconds (files took 11.15 seconds to load)
5 examples, 1 failure

Failed examples:

rspec ./spec/integration/sentry_spec.rb:47 # config/initializers/sentry when an exception is raised containing personally identifying information replaces the email address in the exception with a comment

Ruby Version

3.3.0

SDK Version

5.15.2

Integration and Its Version

Rails

Sentry Config

https://github.com/Laings-Online-Limited/sentry-ruby-3.3-bug/blob/main/config/initializers/sentry.rb

require "active_support/parameter_filter"

require "./app/lib/email_parameter_filter_proc"

if Settings.sentry.dsn.present?
  Sentry.init do |config|
    config.dsn = Settings.sentry.dsn
    config.breadcrumbs_logger = %i[active_support_logger http_logger]
    config.debug = true
    config.enable_tracing = false
    config.environment = Settings.sentry.environment

    # use synchronous/blocking code for integration tests
    config.background_worker_threads = 0 if Rails.env.test?

    filter = ActiveSupport::ParameterFilter.new(
      [EmailParameterFilterProc.new(mask: Settings.sentry.filter_mask)],
      mask: Settings.sentry.filter_mask,
    )
    config.before_send = lambda do |event, _hint|
      filter.filter(event.to_hash)
    end
  end
end
sl0thentr0py commented 10 months ago

thanks for the detailed repro @aliuk2012 we still haven't officially tried out 3.3 but I'll take a look soon

sl0thentr0py commented 10 months ago

ok so this is something about how the exception is formatting the SupportForm variable that changed

➜  sentry-ruby-3.3-bug git:(main) ✗ rvm use 3.2 && bundle exec rspec spec/integration/sentry_spec.rb
Using /Users/neel/.rvm/gems/ruby-3.2.2
.undefined method `not_a_method' for #<SupportForm:0x000000010731eb30 @id=1, @submission_email="[Filtered (client-side)]"> (NoMethodError)

      form.not_a_method
          ^^^^^^^^^^^^^
➜  sentry-ruby-3.3-bug git:(main) ✗ rvm use 3.3 && bundle exec rspec spec/integration/sentry_spec.rb
Using /Users/neel/.rvm/gems/ruby-3.3.0
.undefined method `not_a_method' for an instance of SupportForm (NoMethodError)

      form.not_a_method
          ^^^^^^^^^^^^^
sl0thentr0py commented 10 months ago

from https://github.com/ruby/ruby/blob/v3_3_0/NEWS.md#compatibility-issues

* Error message for NoMethodError have changed to not use the target object's `#inspect`
  for efficiency, and says "instance of ClassName" instead. [[Feature #18285]]

    ```ruby
    ([1] * 100).nonexisting
    # undefined method `nonexisting' for an instance of Array (NoMethodError)


so I'd just ask you to fix your test, this behavior is ok for us and is decided by upstream.
aliuk2012 commented 10 months ago

@sl0thentr0py thank you so much for taking a look. If I understand you correctly in this instance Sentry would only log the name of the object and not all the details of the object itself?

Are there any exceptions that Sentry would inspect the object and log it? Our goal is to try to scrub sensitive information that an object might have before it gets to Sentry.

sl0thentr0py commented 10 months ago

well it was just ruby inspecting the object and not us and now they removed it so it is safer than before