From bed5b91410c67728c1e3899a8de373689c6a7d30 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Thu, 23 Jul 2026 14:00:40 +0200 Subject: [PATCH] feat(data-collection): Port Rails controller and ActiveStorage * Rails controller span data query inclusion is modified to comply with URL collection spec. * Rails `filter_parameters` are backfilled into `url_query_params` * Allow regexes in `terms` as a result to enable this --- .../lib/sentry/rails/configuration.rb | 8 +++ .../sentry/rails/controller_transaction.rb | 15 ++++- .../tracing/active_storage_subscriber.rb | 5 +- .../test_rails_app/config/application.rb | 4 +- .../tracing/active_storage_subscriber_spec.rb | 48 +++++++++++++++ .../spec/sentry/rails/tracing_spec.rb | 60 ++++++++++++++++--- sentry-rails/spec/sentry/rails_spec.rb | 32 ++++++++++ .../data_collection/key_value_collection.rb | 18 +++--- sentry-ruby/lib/sentry/utils/http_tracing.rb | 4 +- .../key_value_collection_spec.rb | 16 +++++ .../spec/sentry/utils/http_tracing_spec.rb | 2 +- 11 files changed, 188 insertions(+), 24 deletions(-) diff --git a/sentry-rails/lib/sentry/rails/configuration.rb b/sentry-rails/lib/sentry/rails/configuration.rb index 9e4c1e7ef..9dc89fd86 100644 --- a/sentry-rails/lib/sentry/rails/configuration.rb +++ b/sentry-rails/lib/sentry/rails/configuration.rb @@ -36,6 +36,14 @@ class Configuration after(:configured) do rails.structured_logging.enabled = enable_logs if rails.structured_logging.enabled.nil? + + collection = data_collection.url_query_params + if collection.mode == :deny_list + filter_parameters = ::Rails.application.config.filter_parameters.select do |filter| + filter.is_a?(String) || filter.is_a?(Symbol) || filter.is_a?(Regexp) + end + collection.terms = (Array(collection.terms) + filter_parameters).uniq + end end end diff --git a/sentry-rails/lib/sentry/rails/controller_transaction.rb b/sentry-rails/lib/sentry/rails/controller_transaction.rb index a753f06d7..db0d394a0 100644 --- a/sentry-rails/lib/sentry/rails/controller_transaction.rb +++ b/sentry-rails/lib/sentry/rails/controller_transaction.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "sentry/utils/http_tracing" + module Sentry module Rails module ControllerTransaction @@ -24,9 +26,16 @@ def sentry_around_action child_span.set_data(:format, request.format) child_span.set_data(:method, request.method) - pii = Sentry.configuration.send_default_pii - child_span.set_data(:path, pii ? request.fullpath : request.filtered_path) - child_span.set_data(:params, pii ? request.params : request.filtered_parameters) + data_collection = Sentry.configuration.data_collection + query = data_collection.url_query_params.filter(request.query_parameters) + path = request.path + path = "#{path}?#{Sentry::Utils::HttpTracing.format_query(query)}" unless query.empty? + + child_span.set_data(:path, path) + child_span.set_data( + :params, + data_collection.url_query_params.filter(request.params) + ) end result diff --git a/sentry-rails/lib/sentry/rails/tracing/active_storage_subscriber.rb b/sentry-rails/lib/sentry/rails/tracing/active_storage_subscriber.rb index fcaa7241a..7786a31cf 100644 --- a/sentry-rails/lib/sentry/rails/tracing/active_storage_subscriber.rb +++ b/sentry-rails/lib/sentry/rails/tracing/active_storage_subscriber.rb @@ -34,7 +34,10 @@ def self.subscribe! ) do |span| payload.each do |key, value| next if key == START_TIMESTAMP_NAME - next if key == :key && !Sentry.configuration.send_default_pii + # Active Storage keys are automatically generated storage data, + # rather than URL/query data. Use the database query-data switch + # for the legacy PII-compatible on/off behavior. + next if key == :key && !Sentry.configuration.data_collection.database_query_data span.set_data(key, value) end diff --git a/sentry-rails/spec/dummy/test_rails_app/config/application.rb b/sentry-rails/spec/dummy/test_rails_app/config/application.rb index a1b68eeda..f0c5c58e6 100644 --- a/sentry-rails/spec/dummy/test_rails_app/config/application.rb +++ b/sentry-rails/spec/dummy/test_rails_app/config/application.rb @@ -109,8 +109,10 @@ def configure :custom_secret, :api_key, :credit_card, + /billing_reference/, :authorization, - :token + :token, + proc { |_key, _value| } ] # Eager load namespaces can be accumulated after repeated initializations and make initialization diff --git a/sentry-rails/spec/sentry/rails/tracing/active_storage_subscriber_spec.rb b/sentry-rails/spec/sentry/rails/tracing/active_storage_subscriber_spec.rb index ec74550f3..e77d041fa 100644 --- a/sentry-rails/spec/sentry/rails/tracing/active_storage_subscriber_spec.rb +++ b/sentry-rails/spec/sentry/rails/tracing/active_storage_subscriber_spec.rb @@ -79,6 +79,54 @@ end end + context "data collection" do + context "with database query data enabled" do + before do + make_basic_app do |config| + config.traces_sample_rate = 1.0 + config.rails.tracing_subscribers = [described_class] + config.data_collection.database_query_data = true + end + end + + it "records the :key in span.data" do + ActiveStorage::AnalyzeJob.queue_adapter.perform_enqueued_jobs = true + + p = Post.create! + get "/posts/#{p.id}/attach" + + request_transaction = transport.events.last.to_h + upload_span = request_transaction[:spans].find { |s| s[:op] == "file.service_upload.active_storage" } + + expect(upload_span).not_to be_nil + expect(upload_span.dig(:data, :key)).to eq(p.cover.key) + end + end + + context "with database query data disabled" do + before do + make_basic_app do |config| + config.traces_sample_rate = 1.0 + config.rails.tracing_subscribers = [described_class] + config.data_collection.database_query_data = false + end + end + + it "does not record the :key in span.data" do + ActiveStorage::AnalyzeJob.queue_adapter.perform_enqueued_jobs = true + + p = Post.create! + get "/posts/#{p.id}/attach" + + request_transaction = transport.events.last.to_h + upload_span = request_transaction[:spans].find { |s| s[:op] == "file.service_upload.active_storage" } + + expect(upload_span).not_to be_nil + expect(upload_span.dig(:data, :key)).to be_nil + end + end + end + context "when transaction is not sampled" do before do make_basic_app diff --git a/sentry-rails/spec/sentry/rails/tracing_spec.rb b/sentry-rails/spec/sentry/rails/tracing_spec.rb index 65988bc88..0aed6810f 100644 --- a/sentry-rails/spec/sentry/rails/tracing_spec.rb +++ b/sentry-rails/spec/sentry/rails/tracing_spec.rb @@ -201,17 +201,15 @@ end end - it "does not record sensitive params" do + it "does not record any params" do get "/posts?foo=bar&password=42&secret=baz" transaction = transport.events.last.to_h params = transaction[:spans][0][:data][:params] - expect(params["foo"]).to eq("bar") - expect(params["password"]).to eq("[FILTERED]") - expect(params["secret"]).to eq("[FILTERED]") + expect(params).to be_empty path = transaction[:spans][0][:data][:path] - expect(path).to eq("/posts?foo=bar&password=[FILTERED]&secret=[FILTERED]") + expect(path).to eq("/posts") end end @@ -223,17 +221,61 @@ end end - it "records all params" do + it "records all params except sensitive params" do get "/posts?foo=bar&password=42&secret=baz" transaction = transport.events.last.to_h params = transaction[:spans][0][:data][:params] expect(params["foo"]).to eq("bar") - expect(params["password"]).to eq("42") - expect(params["secret"]).to eq("baz") + expect(params["password"]).to eq("[Filtered]") + expect(params["secret"]).to eq("[Filtered]") + + path = transaction[:spans][0][:data][:path] + expect(path).to eq("/posts?foo=bar&password=[Filtered]&secret=[Filtered]") + end + end + end + + context "data collection" do + context "with url query params enabled" do + before do + make_basic_app do |config| + config.traces_sample_rate = 1.0 + config.data_collection.url_query_params.mode = :deny_list + end + end + + it "records query params except sensitive params" do + get "/posts?foo=bar&password=42&secret=baz" + transaction = transport.events.last.to_h + + params = transaction[:spans][0][:data][:params] + expect(params["foo"]).to eq("bar") + expect(params["password"]).to eq("[Filtered]") + expect(params["secret"]).to eq("[Filtered]") + + path = transaction[:spans][0][:data][:path] + expect(path).to eq("/posts?foo=bar&password=[Filtered]&secret=[Filtered]") + end + end + + context "with url query params disabled" do + before do + make_basic_app do |config| + config.traces_sample_rate = 1.0 + config.data_collection.url_query_params.mode = :off + end + end + + it "does not record query params" do + get "/posts?foo=bar&password=42&secret=baz" + transaction = transport.events.last.to_h + + params = transaction[:spans][0][:data][:params] + expect(params).to be_empty path = transaction[:spans][0][:data][:path] - expect(path).to eq("/posts?foo=bar&password=42&secret=baz") + expect(path).to eq("/posts") end end end diff --git a/sentry-rails/spec/sentry/rails_spec.rb b/sentry-rails/spec/sentry/rails_spec.rb index 1678a044f..762c6d59c 100644 --- a/sentry-rails/spec/sentry/rails_spec.rb +++ b/sentry-rails/spec/sentry/rails_spec.rb @@ -105,6 +105,38 @@ end end + context "data collection" do + before do + make_basic_app do |config| + config.data_collection.url_query_params.mode = :deny_list + Rails.application.config.filter_parameters << proc { |_key, _value| } + end + end + + it "adds Rails filter parameters to URL query parameter data collection" do + expect(Sentry.configuration.data_collection.url_query_params.terms).to include( + "password", + "custom_secret" + ) + end + + it "preserves and applies Regexp Rails filter parameters" do + expect(Sentry.configuration.data_collection.url_query_params.terms).to include(/billing_reference/) + expect(Sentry.configuration.data_collection.url_query_params.filter( + { "billing_reference_id" => "secret", "public" => "visible" } + )).to eq( + "billing_reference_id" => "[Filtered]", + "public" => "visible" + ) + end + + it "only adds String, Symbol, and Regexp Rails filter parameters" do + terms = Sentry.configuration.data_collection.url_query_params.terms + + expect(terms).to all(satisfy { |term| term.is_a?(String) || term.is_a?(Symbol) || term.is_a?(Regexp) }) + end + end + context "at exit" do before do make_basic_app diff --git a/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb b/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb index 14f0502d9..ed911c887 100644 --- a/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb +++ b/sentry-ruby/lib/sentry/data_collection/key_value_collection.rb @@ -61,7 +61,7 @@ class KeyValueCollection attr_accessor :mode # `terms` contains the keys or patterns used by the selected mode. - # @return [Array, nil] + # @return [Array, nil] attr_reader :terms def initialize(mode:, terms:) @@ -70,7 +70,8 @@ def initialize(mode:, terms:) end def terms=(terms) - @terms = terms&.map { |term| term.to_s.downcase }&.reject { |term| term.strip.empty? } + @terms = terms&.map { |term| term.is_a?(Regexp) ? term : term.to_s.downcase } + &.reject { |term| !term.is_a?(Regexp) && term.strip.empty? } end # Applies this collection configuration without changing the input hash. @@ -90,14 +91,15 @@ def filter(values, cookie: false) private def safe_value?(key, cookie: false) - key_downcase = key.to_s.downcase + key_string = key.to_s + key_downcase = key_string.downcase return false if sensitive?(key_downcase, cookie: cookie) case mode when :deny_list - !matches_any_term?(key_downcase) + !matches_any_term?(key_string, key_downcase) when :allow_list - matches_any_term?(key_downcase) + matches_any_term?(key_string, key_downcase) else false end @@ -108,8 +110,10 @@ def sensitive?(key, cookie: false) (cookie && SENSITIVE_COOKIE_NAME_DENY_LIST.any? { |term| key.include?(term) }) end - def matches_any_term?(key) - @terms&.any? { |term| key.include?(term) } + def matches_any_term?(key, key_downcase) + @terms&.any? do |term| + term.is_a?(Regexp) ? term.match?(key) : key_downcase.include?(term) + end end end end diff --git a/sentry-ruby/lib/sentry/utils/http_tracing.rb b/sentry-ruby/lib/sentry/utils/http_tracing.rb index e89a3cc7b..0e6884433 100644 --- a/sentry-ruby/lib/sentry/utils/http_tracing.rb +++ b/sentry-ruby/lib/sentry/utils/http_tracing.rb @@ -20,12 +20,12 @@ def filter_query_params(query) filtered_query_hash = Sentry.configuration.data_collection.url_query_params.filter(query_hash) return nil if filtered_query_hash.empty? - format_query(filtered_query_hash) + HttpTracing.format_query(filtered_query_hash) rescue nil end - def format_query(query) + def self.format_query(query) query.flat_map do |key, value| Array(value).map { |item| "#{key}=#{item}" } end.join("&") diff --git a/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb b/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb index 4b0f1ce97..0d720bbcd 100644 --- a/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb +++ b/sentry-ruby/spec/sentry/data_collection/key_value_collection_spec.rb @@ -54,6 +54,22 @@ ) end + it "matches regular expression terms" do + expect(described_class.new(mode: :deny_list, terms: [/private[-_]data/]).filter( + { "private_data" => "secret", "public_data" => "visible" } + )).to eq( + "private_data" => "[Filtered]", + "public_data" => "visible" + ) + end + + it "preserves regular expression terms when assigning terms" do + regexp = /private/i + collection.terms = [regexp] + + expect(collection.terms).to eq([regexp]) + end + it "normalizes terms assigned after initialization" do collection.terms = ["USER"] diff --git a/sentry-ruby/spec/sentry/utils/http_tracing_spec.rb b/sentry-ruby/spec/sentry/utils/http_tracing_spec.rb index 769285732..198dcd4b8 100644 --- a/sentry-ruby/spec/sentry/utils/http_tracing_spec.rb +++ b/sentry-ruby/spec/sentry/utils/http_tracing_spec.rb @@ -21,7 +21,7 @@ "tags[]" => ["ruby", "sentry"] } - expect(http_tracing.format_query(query)).to eq( + expect(described_class.format_query(query)).to eq( "token=[Filtered]&page=5&tags[]=ruby&tags[]=sentry" ) end