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.

Sign up for GitHub

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

Redact URLs in automatic Rails breadcrumbs #806

Merged
merged 1 commit into from
Dec 18, 2023
+13 −4
Merged

Redact URLs in automatic Rails breadcrumbs #806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ Changelog
* Fix Resque integration when failure backend is already `Resque::Failure::Multiple`
| [#803](https://github.com/bugsnag/bugsnag-ruby/pull/803)
| [sj26](https://github.com/sj26)
* Redact URLs in automatic Rails breadcrumbs
| [#806](https://github.com/bugsnag/bugsnag-ruby/pull/806)

## v6.26.0 (19 July 2023)

Expand Down
4 changes: 2 additions & 2 deletions features/fixtures/expected_breadcrumbs/request.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
"controller": "BreadcrumbsController",
"action": "handled",
"method": "GET",
"path": "/breadcrumbs/handled",
"path": "/breadcrumbs/handled?password=[FILTERED]&abc=xyz",
"event_name": "start_processing.action_controller",
"event_id": ".*"
}
}
}
2 changes: 1 addition & 1 deletion features/rails_features/breadcrumbs.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Feature: Rails automatic breadcrumbs
@rails3 @rails4 @rails5 @rails6 @rails7
Scenario: Request breadcrumb
Given I start the rails service
When I navigate to the route "/breadcrumbs/handled" on the rails app
When I navigate to the route "/breadcrumbs/handled?password=secret&abc=xyz" on the rails app
And I wait to receive an error
Then the error is valid for the error reporting API version "4.0" for the "Ruby Bugsnag Notifier" notifier
And the event contains a breadcrumb matching the JSON fixture in "features/fixtures/expected_breadcrumbs/request.json"
Expand Down
9 changes: 8 additions & 1 deletion lib/bugsnag/integrations/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ def event_subscription(event)
filtered_data[:event_name] = event[:id]
filtered_data[:event_id] = event_id

if event[:id] == "sql.active_record"
case event[:id]
when "sql.active_record"
if data.key?(:binds)
binds = data[:binds].each_with_object({}) { |bind, output| output[bind.name] = '?' if defined?(bind.name) }
filtered_data[:binds] = JSON.dump(binds) unless binds.empty?
Expand All @@ -36,6 +37,12 @@ def event_subscription(event)
# the connection ID is the object_id of the connection object
filtered_data[:connection_id] = data[:connection].object_id
end

when "start_processing.action_controller"
filtered_data[:path] = Bugsnag.cleaner.clean_url(data[:path]) if data.key?(:path)

when "redirect_to.action_controller"
filtered_data[:location] = Bugsnag.cleaner.clean_url(data[:location]) if data.key?(:location)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes an error to surface in the app if the data[:location] (ie. the redirect_to url) is a badly formed, but actually in practice working URL, such as: https://example.com/some path with spaces -- browsers (and even rack:test) will automatically convert this to https://example.com/some path%20with%20spaces but URI("https://example.com/some path with spaces") (as used at https://github.com/bugsnag/bugsnag-ruby/blob/master/lib/bugsnag/cleaner.rb#L30) will raise URI::InvalidURIError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @timdiggins! I'll try to get a release out with a fix early next week

end

Bugsnag.leave_breadcrumb(
Expand Down
Loading