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

Author: sl0thentr0py

sentry-rails/lib/sentry/rails/controller_transaction.rb

@@ -1,10 +1,14 @@
 # frozen_string_literal: true
 
+require "sentry/utils/http_tracing"
+
 module Sentry
   module Rails
     module ControllerTransaction
       SPAN_ORIGIN = "auto.view.rails"
 
+      include Sentry::Utils::HttpTracing
+
       def self.included(base)
         base.prepend_around_action(:sentry_around_action)
       end
@@ -24,9 +28,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}?#{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

sentry-rails/lib/sentry/rails/railtie.rb

@@ -48,6 +48,7 @@ class Railtie < ::Rails::Railtie
       next unless Sentry.initialized?
 
       configure_project_root
+      configure_data_collection
       configure_trusted_proxies
       configure_cron_timezone
       extend_controller_methods if defined?(ActionController)
@@ -82,6 +83,14 @@ def configure_project_root
       Sentry.configuration.project_root = ::Rails.root.to_s
     end
 
+    def configure_data_collection
+      collection = Sentry.configuration.data_collection.url_query_params
+      return unless collection.mode == :deny_list
+
+      filter_parameters = ::Rails.application.config.filter_parameters
+      collection.terms = Array(collection.terms) + Array(filter_parameters)
+    end
+
     def configure_trusted_proxies
       Sentry.configuration.trusted_proxies += Array(::Rails.application.config.action_dispatch.trusted_proxies)
     end

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

sentry-rails/spec/dummy/test_rails_app/config/application.rb

@@ -109,6 +109,7 @@ def configure
             :custom_secret,
             :api_key,
             :credit_card,
+            /billing_reference/,
             :authorization,
             :token
           ]

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

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

sentry-rails/spec/sentry/rails_spec.rb

@@ -105,6 +105,31 @@
     end
   end
 
+  context "data collection" do
+    before do
+      make_basic_app do |config|
+        config.data_collection.url_query_params.mode = :deny_list
+      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
+  end
+
   context "at exit" do
     before do
       make_basic_app

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<String>, nil]
+      # @return [Array<String, Regexp>, 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

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"]