From e5e79d9aa17006a6995e9ea18fabdc14a2356c82 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Tue, 27 Jun 2023 08:15:12 +0200 Subject: [PATCH] Add filter_metadata config option (#973) 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. This change allow apps to filter out some metadata that's set by default, like `path`, to avoid sending PII or other sensitive data, using the `filter_metadata` config option. Filtering is done with String based keys, like all the other `filter_*` config options are, so the keys need to be transformed to keys beforehand to make sure they're filtered out. I didn't merge how we set the metadata, now it's set using `Transaction#set_metadata` and through `sample_data` when the Transaction is being sampled as sample data. I've left the behavior the same as much as possible to avoid breaking things. See also this internal discussion: https://appsignal.slack.com/archives/CNPP953E2/p1687785270464119 --- .../add-filter_metadata-config-option.md | 6 ++++ lib/appsignal/config.rb | 3 ++ lib/appsignal/transaction.rb | 16 ++++++---- spec/integration/diagnose | 2 +- spec/lib/appsignal/config_spec.rb | 1 + .../appsignal/rack/streaming_listener_spec.rb | 1 + spec/lib/appsignal/transaction_spec.rb | 29 ++++++++++++++++--- 7 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 .changesets/add-filter_metadata-config-option.md diff --git a/.changesets/add-filter_metadata-config-option.md b/.changesets/add-filter_metadata-config-option.md new file mode 100644 index 000000000..6c9a5018d --- /dev/null +++ b/.changesets/add-filter_metadata-config-option.md @@ -0,0 +1,6 @@ +--- +bump: "patch" +type: "add" +--- + +Add `filter_metadata` config option to filter metadata set on Transactions set by default. Metadata like `path`, (request) `method`, `request_id`, `hostname`, etc. This can be useful if there's PII or other sensitive data in any of the app's metadata. diff --git a/lib/appsignal/config.rb b/lib/appsignal/config.rb index c889b049d..2f0cc8b14 100644 --- a/lib/appsignal/config.rb +++ b/lib/appsignal/config.rb @@ -24,6 +24,7 @@ class Config :enable_rails_error_reporter => true, :endpoint => "https://push.appsignal.com", :files_world_accessible => true, + :filter_metadata => [], :filter_parameters => [], :filter_session_data => [], :ignore_actions => [], @@ -77,6 +78,7 @@ class Config "APPSIGNAL_ENABLE_GVL_WAITING_THREADS" => :enable_gvl_waiting_threads, "APPSIGNAL_ENABLE_RAILS_ERROR_REPORTER" => :enable_rails_error_reporter, "APPSIGNAL_FILES_WORLD_ACCESSIBLE" => :files_world_accessible, + "APPSIGNAL_FILTER_METADATA" => :filter_metadata, "APPSIGNAL_FILTER_PARAMETERS" => :filter_parameters, "APPSIGNAL_FILTER_SESSION_DATA" => :filter_session_data, "APPSIGNAL_HOSTNAME" => :hostname, @@ -150,6 +152,7 @@ class Config # @api private ENV_ARRAY_KEYS = %w[ APPSIGNAL_DNS_SERVERS + APPSIGNAL_FILTER_METADATA APPSIGNAL_FILTER_PARAMETERS APPSIGNAL_FILTER_SESSION_DATA APPSIGNAL_IGNORE_ACTIONS diff --git a/lib/appsignal/transaction.rb b/lib/appsignal/transaction.rb index 2cc723fde..402f97648 100644 --- a/lib/appsignal/transaction.rb +++ b/lib/appsignal/transaction.rb @@ -308,6 +308,7 @@ def set_http_or_background_queue_start def set_metadata(key, value) return unless key && value + return if Appsignal.config[:filter_metadata].include?(key.to_s) @ext.set_metadata(key, value) end @@ -337,7 +338,7 @@ def sample_data :params => sanitized_params, :environment => sanitized_environment, :session_data => sanitized_session_data, - :metadata => metadata, + :metadata => sanitized_metadata, :tags => sanitized_tags, :breadcrumbs => breadcrumbs }.each do |key, data| @@ -522,12 +523,17 @@ def sanitized_session_data ) end - # Returns metadata from the environment. + # Returns sanitized metadata set by {#set_metadata} and from the + # {#environment}. # - # @return [nil] if no `:metadata` key is present in the {#environment}. # @return [Hash] - def metadata - environment[:metadata] + def sanitized_metadata + metadata = environment[:metadata] + return unless metadata + + metadata + .transform_keys(&:to_s) + .except(*Appsignal.config[:filter_metadata]) end # Returns the environment for a transaction. diff --git a/spec/integration/diagnose b/spec/integration/diagnose index 994514807..b70c5a12c 160000 --- a/spec/integration/diagnose +++ b/spec/integration/diagnose @@ -1 +1 @@ -Subproject commit 994514807b4c902587a199dfb315754e613372c3 +Subproject commit b70c5a12c15e5a68a803a1e3a62620444233468a diff --git a/spec/lib/appsignal/config_spec.rb b/spec/lib/appsignal/config_spec.rb index e3f94fe08..db22447a2 100644 --- a/spec/lib/appsignal/config_spec.rb +++ b/spec/lib/appsignal/config_spec.rb @@ -165,6 +165,7 @@ :enable_rails_error_reporter => true, :endpoint => "https://push.appsignal.com", :files_world_accessible => true, + :filter_metadata => [], :filter_parameters => [], :filter_session_data => [], :ignore_actions => [], diff --git a/spec/lib/appsignal/rack/streaming_listener_spec.rb b/spec/lib/appsignal/rack/streaming_listener_spec.rb index efe65dfac..4b6d7fa82 100644 --- a/spec/lib/appsignal/rack/streaming_listener_spec.rb +++ b/spec/lib/appsignal/rack/streaming_listener_spec.rb @@ -1,6 +1,7 @@ require "appsignal/rack/streaming_listener" describe Appsignal::Rack::StreamingListener do + before(:context) { start_agent } let(:headers) { {} } let(:env) do { diff --git a/spec/lib/appsignal/transaction_spec.rb b/spec/lib/appsignal/transaction_spec.rb index c338763f5..c825be0f7 100644 --- a/spec/lib/appsignal/transaction_spec.rb +++ b/spec/lib/appsignal/transaction_spec.rb @@ -623,6 +623,18 @@ def current_transaction expect(transaction.to_h["metadata"]).to eq("request_method" => "GET") end + context "when filter_metadata includes metadata key" do + before { Appsignal.config[:filter_metadata] = ["filter_key"] } + after { Appsignal.config[:filter_metadata] = [] } + + it "does not set the metadata on the transaction" do + transaction.set_metadata(:filter_key, "filtered value") + transaction.set_metadata("filter_key", "filtered value") + + expect(transaction.to_h["metadata"].keys).to_not include("filter_key") + end + end + context "when the key is nil" do it "does not update the metadata on the transaction" do transaction.set_metadata(nil, "GET") @@ -1275,8 +1287,8 @@ def session_exists?(_env) end end - describe "#metadata" do - subject { transaction.send(:metadata) } + describe "#sanitized_metadata" do + subject { transaction.send(:sanitized_metadata) } context "when request is nil" do let(:request) { nil } @@ -1291,9 +1303,18 @@ def session_exists?(_env) end context "when env is present" do - let(:env) { { :metadata => { :key => "value" } } } + let(:env) { { "key" => "value" } } - it { is_expected.to eq env[:metadata] } + it { is_expected.to eq("key" => "value") } + + context "with filter_metadata option set" do + before { Appsignal.config[:filter_metadata] = ["key"] } + after { Appsignal.config[:filter_metadata] = [] } + + it "filters out keys listed in the filter_metadata option" do + expect(subject.keys).to_not include("key") + end + end end end