bugsnag / bugsnag-ruby

BugSnag error monitoring & reporting software for rails, sinatra, rack and ruby
https://docs.bugsnag.com/platforms/ruby
MIT License
246 stars 174 forks source link

Fix unhandled `URI::InvalidURIError` in `Cleaner#clean_url` #811

Closed imjoehaines closed 6 months ago

imjoehaines commented 6 months ago

Goal

As reported in #810, we don't handle invalid URLs in clean_url so raise if the URL isn't valid

If the URL doesn't have a query string then it's not important that it fails validation and so we can return it as-is

If the URL does have a query string and cannot be parsed, we can't redact parameters individually and so redact the whole query string instead — this way the URL could still be useful and we don't risk leaking sensitive data

stevenharman commented 6 months ago

Ooof, this was also causing a memory leak on our app due to the way older Rails (6.1 and 7.0, at least) manage their ActiveSupport::Subscriber array.

Basically, each ActiveSupport::Notifications.subscribe block pushes an ActiveSupport::Notifications::Event onto the ActiveSupport::Subscriber#event_stack. Those events are then popped off in the reverse order (last-in, first out) and processed. The processing of such Events is the mechanism used by Bugsnag to scrub those URLs/paths. When that mechanism raises an error, as was happening here, in an inner Event, the outer Events are left on the event_stack. Forever. Thus leaking memory.

This PR fixes that, and the entire event_stack has been removed in Rails 7.1. Phew!