From e779f1e3261e4f443252e9506a935da5c5b680a7 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Sat, 24 Jun 2023 10:01:51 +0200 Subject: [PATCH] Add option to set route definition as Sinatra path 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-sinatra_sanitized_routes-option.md | 6 ++++++ lib/appsignal/config.rb | 3 +++ lib/appsignal/rack/sinatra_instrumentation.rb | 15 ++++++++++++++- spec/integration/diagnose | 2 +- spec/lib/appsignal/config_spec.rb | 1 + .../rack/sinatra_instrumentation_spec.rb | 18 ++++++++++++++++++ 6 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 .changesets/add-sinatra_sanitized_routes-option.md diff --git a/.changesets/add-sinatra_sanitized_routes-option.md b/.changesets/add-sinatra_sanitized_routes-option.md new file mode 100644 index 000000000..5fd678695 --- /dev/null +++ b/.changesets/add-sinatra_sanitized_routes-option.md @@ -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. diff --git a/lib/appsignal/config.rb b/lib/appsignal/config.rb index c889b049d..9055a71f6 100644 --- a/lib/appsignal/config.rb +++ b/lib/appsignal/config.rb @@ -44,6 +44,7 @@ class Config ], :send_environment_metadata => true, :send_params => true, + :sinatra_sanitized_routes => false, :transaction_debug_mode => false }.freeze @@ -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, @@ -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 diff --git a/lib/appsignal/rack/sinatra_instrumentation.rb b/lib/appsignal/rack/sinatra_instrumentation.rb index 678052c9a..be4aaa882 100644 --- a/lib/appsignal/rack/sinatra_instrumentation.rb +++ b/lib/appsignal/rack/sinatra_instrumentation.rb @@ -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) transaction.set_metadata("method", request.request_method) transaction.set_http_or_background_queue_start Appsignal::Transaction.complete_current! @@ -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) && diff --git a/spec/integration/diagnose b/spec/integration/diagnose index 994514807..19cfce561 160000 --- a/spec/integration/diagnose +++ b/spec/integration/diagnose @@ -1 +1 @@ -Subproject commit 994514807b4c902587a199dfb315754e613372c3 +Subproject commit 19cfce561345318a7f892107c3eec01ee3eb70de diff --git a/spec/lib/appsignal/config_spec.rb b/spec/lib/appsignal/config_spec.rb index e3f94fe08..5032ba9a8 100644 --- a/spec/lib/appsignal/config_spec.rb +++ b/spec/lib/appsignal/config_spec.rb @@ -183,6 +183,7 @@ :send_environment_metadata => true, :send_params => true, :send_session_data => true, + :sinatra_sanitized_routes => false, :transaction_debug_mode => false ) end diff --git a/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb b/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb index 279b45a9b..8805d7d4b 100644 --- a/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb +++ b/spec/lib/appsignal/rack/sinatra_instrumentation_spec.rb @@ -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