Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

imjoehaines
Copy link
Member

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

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
@imjoehaines imjoehaines merged commit c052209 into master Jan 17, 2024
137 checks passed
@imjoehaines imjoehaines deleted the fix-uri-error branch January 17, 2024 08:36
@stevenharman
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants