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

Add option to set route definition as Sinatra path #972

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changesets/add-sinatra_sanitized_routes-option.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: "patch"
type: "add"
---

Add sinatra_sanitized_routes option to store sanitized routes instead of the real request path in the Transaction's metadata. This can be useful if there's PII or other sensitive data in the app's request paths. Set `sinatra_sanitized_routes` to `true` in the AppSignal config to enable this behavior and store the route definition rather than the real request path as metadata. We also recommend changing the `request_headers` config option to not include any headers that also include the real request path.
3 changes: 3 additions & 0 deletions lib/appsignal/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class Config
],
:send_environment_metadata => true,
:send_params => true,
:sinatra_sanitized_routes => false,
:transaction_debug_mode => false
}.freeze

Expand Down Expand Up @@ -99,6 +100,7 @@ class Config
"APPSIGNAL_SEND_ENVIRONMENT_METADATA" => :send_environment_metadata,
"APPSIGNAL_SEND_PARAMS" => :send_params,
"APPSIGNAL_SEND_SESSION_DATA" => :send_session_data,
"APPSIGNAL_SINATRA_SANITIZED_ROUTES" => :sinatra_sanitized_routes,
"APPSIGNAL_SKIP_SESSION_DATA" => :skip_session_data,
"APPSIGNAL_STATSD_PORT" => :statsd_port,
"APPSIGNAL_TRANSACTION_DEBUG_MODE" => :transaction_debug_mode,
Expand Down Expand Up @@ -144,6 +146,7 @@ class Config
APPSIGNAL_SEND_ENVIRONMENT_METADATA
APPSIGNAL_SEND_PARAMS
APPSIGNAL_SEND_SESSION_DATA
APPSIGNAL_SINATRA_SANITIZED_ROUTES
APPSIGNAL_SKIP_SESSION_DATA
APPSIGNAL_TRANSACTION_DEBUG_MODE
].freeze
Expand Down
15 changes: 14 additions & 1 deletion lib/appsignal/rack/sinatra_instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,13 @@ def call_with_appsignal_monitoring(env)
transaction.set_error(env["sinatra.error"])
end
transaction.set_action_if_nil(action_name(env))
transaction.set_metadata("path", request.path)
path =
if Appsignal.config[:sinatra_sanitized_routes]
sanitized_route(env)
else
request.path
end
transaction.set_metadata("path", path)
Comment on lines +73 to +79
Copy link
Contributor

@unflxw unflxw Jun 26, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have a look if I can add a filter_metadata option really quickly to accomplish the same thing for all instrumentations.

transaction.set_metadata("method", request.request_method)
transaction.set_http_or_background_queue_start
Appsignal::Transaction.complete_current!
Expand All @@ -90,6 +96,13 @@ def action_name(env)

private

def sanitized_route(env)
return unless env["sinatra.route"]

_method, route = env["sinatra.route"].split
route
end

def raise_errors?(app)
app.respond_to?(:settings) &&
app.settings.respond_to?(:raise_errors) &&
Expand Down
2 changes: 1 addition & 1 deletion spec/integration/diagnose
1 change: 1 addition & 0 deletions spec/lib/appsignal/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@
:send_environment_metadata => true,
:send_params => true,
:send_session_data => true,
:sinatra_sanitized_routes => false,
:transaction_debug_mode => false
)
end
Expand Down
18 changes: 18 additions & 0 deletions spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,24 @@ def make_request_with_error(env, error)
)
)
end

context "with sinatra_sanitized_routes=true" do
before { Appsignal.config[:sinatra_sanitized_routes] = true }
after { Appsignal.config[:sinatra_sanitized_routes] = false }
let(:env) { { "sinatra.route" => "GET /some/:path", "REQUEST_METHOD" => "GET" } }

it "sets the path as the sanitized defined route" do
make_request(env)

expect(created_transactions.count).to eq(1)
expect(last_transaction.to_h).to include(
"metadata" => {
"method" => "GET",
"path" => "/some/:path"
}
)
end
end
end

context "with queue start" do
Expand Down