Add option to set route definition as Sinatra path #972
Conversation
We had a report of an app that contains sensitive information in the request path and the desire to filter this out. We have no system in place to filter metadata like path and request method, as set by the Sinatra middleware. Add a config option `sinatra_sanitized_routes` that can be set to `true` to set the route definition rather than the real request path as the `path` metadata.
Add test route to Sinatra app to test the sanitized routes / route definition to be set as the `path` metadata to avoid storing PII and other sensitive data. Related PR appsignal/appsignal-ruby#972
Add test route to Sinatra app to test the sanitized routes / route definition to be set as the `path` metadata to avoid storing PII and other sensitive data. Related PR appsignal/appsignal-ruby#972
|
Related support conversation: https://app.intercom.com/a/apps/yzor8gyw/inbox/all/conversations/16410700216569 |
There was a problem hiding this comment.
As mentioned in the call, almost every instrumentation sends the full path as a tag, so, in the future, we should consider solving this for the general case, rather than by having config options for each of them. Some filter_tags configuration option, like our existing filter_parameters or filter_session_data.
Left a comment about the current implementation, but that said, happy to merge this in any form, as it fixes an issue for an user today. We can always change this and/or deprecate this later.
| path = | ||
| if Appsignal.config[:sinatra_sanitized_routes] | ||
| sanitized_route(env) | ||
| else | ||
| request.path | ||
| end | ||
| transaction.set_metadata("path", path) |
There was a problem hiding this comment.
Perhaps, rather than sending the sanitised route, we shouldn't send path at all when this config option is enabled? I'm not sure if there's value in sending the route as path -- the actions are already being grouped by the route, so filtering by this tag won't do anything interesting.
And then maybe we can name it sinatra_filter_path, which I think would express its purpose more clearly. After all, routes, as in action names, are already sanitised in Sinatra by default.
There was a problem hiding this comment.
I'll have a look if I can add a filter_metadata option really quickly to accomplish the same thing for all instrumentations.
|
Closing in favor of #973 |
Add test route to Sinatra app to test the sanitized routes / route definition to be set as the `path` metadata to avoid storing PII and other sensitive data. Related PR appsignal/appsignal-ruby#972
* Add example app for Sinatra sanitized routes Add test route to Sinatra app to test the sanitized routes / route definition to be set as the `path` metadata to avoid storing PII and other sensitive data. Related PR appsignal/appsignal-ruby#972 * Update config option to filter_metadata Added in PR appsignal/appsignal-ruby#973 in favor of the `sinatra_sanitized_routes` option.
We had a report of an app that contains sensitive information in the request path and the desire to filter this out. We have no system in place to filter metadata like path and request method, as set by the Sinatra middleware.
Add a config option
sinatra_sanitized_routesthat can be set totrueto set the route definition rather than the real request path as thepathmetadata.To do
To do after merge
Based on #971